-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
pds: init at 0.4.67, nixos/pds: init #350645
base: master
Are you sure you want to change the base?
Conversation
f315d95
to
6afa70f
Compare
Upstream uses PNPM, why not use the existing PNPM tooling to generate the package? That means then there's no need to store an additional lockfile in-tree. See https://nixos.org/manual/nixpkgs/stable/#javascript-pnpm for how to use nixpkgs' pnpm tooling. Edit: I see your comment. Let me see if I can improve that for the pnpm tooling. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/4786 |
Upstream has updated to 0.4.67, you might want to consider updating. |
6afa70f
to
0b35498
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small changes
Managed to successfully deploy it to my server :) |
0b35498
to
4065add
Compare
Yeah, I didn't bother packaging the admin scripts because the main one just fetches scripts for subcommands and they all have wrong shebang. Probably upstream should be fixed to use Edit: opened a PR upstream bluesky-social/pds#121 |
You should also be able to run the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work on the service, found a few nits though
Also, having looked into the |
Not really. It'll fix the main script but all it's doing is download subsequent ones that will still fail because they're not patched https://github.com/bluesky-social/pds/blob/main/pdsadmin.sh#L22-L30 Other way would be to patch the main script to use store path instead of downloading it but I wasn't sure if that's not too much change. But maybe that's a good idea to keep them locked to pds version |
yeah I'd say vendor the scripts, you could even make a new pdsadmin script in-tree and replace the one in the repo with it if you want. Removing what's essentially |
ac7f4dc
to
2d67d86
Compare
Hm
on my server |
913a3a8
to
8cabcfb
Compare
Missing shebang 🙃 |
I'll add NixOS test when I'll have more time, deployed on my server and works fine |
8cabcfb
to
bd95c7f
Compare
Now with freeform settings and NixOS test! |
a00d060
to
d4bc1b1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i personally still don't like the idea of switching package managers and maintaining an entirely different lockfile in-tree. this unnecessarily increases maintenance burden and i doubt r-ryantm knows how to update this automatically. please correct me if i am wrong.
otherwise, looks okay. remember to add a release note.
r-ryantm definitely can't update this without an update script. however, the linked upstream pr in one of my comments should help resolve the issue of needing to switch from pnpm(though that still needs to be fully tested once upstream releases a new version of the pds) |
d4bc1b1
to
6e28a70
Compare
Yep. The next bump will need to be manually and tested if new |
6e28a70
to
ba958fa
Compare
ba958fa
to
e7dd394
Compare
one last little nitpick, could there be an option to enable/disable adding the pdsadmin scripts to |
|
e7dd394
to
7e41d44
Compare
Things done
Self hosted server for https://bsky.social/
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.