search

pinpox / lollypops

Lollypop Operations - NixOS Deployment Tool
https://pinpox.github.io/lollypops/
GNU General Public License v3.0
118 stars 17 forks source link

Use `nix flake archive` or `nix copy` #33

Closed GeoffreyFrogeye closed 4 months ago

GeoffreyFrogeye commented 5 months ago

Closes #30

Commit-by-commit review suggested, as the second one might be more controversial.

Regarding its breaking change, IHMO the previous behaviour was problematic, since the user could be left wondering why changes haven't been applied. To make it more user-friendly, we could add a dependency to deploy-flake onrebuild. What do you think?

GeoffreyFrogeye commented 5 months ago

(Fixed NIX_SSHOPTS accidentally removed in the second commit, also added a removed option)

pinpox commented 5 months ago

This PR looks awesome, thank you for working on it! I want to thoroughly test it though before merging, so give me a few days to review please.

I'll also create a tagged release before merging this, so users can still use a pinned version tag in case of any breaking changes.

GeoffreyFrogeye commented 5 months ago

give me a few days to review please

Sure, take your time. The beauty of Nix is that I can use my contributions myself without requiring someone to release them :)

I'll also create a tagged release before merging this

That's a great idea!

pinpox commented 5 months ago

Let me know what you think, I'm open to discussion. In the original issue #30 there is a bit more context/explanation of what I mean:

To rebuild we would need to know that path. A possible solution would be to create a link to the path in the store in the location where the flake was deployed until now. e.g.:

ssh ln -s /nix/store/78vb5imcxk2rla6s13av8vdm5n3rp49m-source /var/src/lollypops/flake

This way, we could the always rebuild like normal by running `nixos-rebuild switch --flake '/var/lib/lollypops/flake'

pinpox commented 5 months ago

@GeoffreyFrogeye I have fixed/implented my comments in a separate branch: https://github.com/pinpox/lollypops/tree/nix-flake-archive-fixes

Could you test them? I think they do all that you want + create a symlink for local evaluation as described above. You can merge it into your PR branch if you want or I can create a new PR from that branch which includes your commits + the fixes. Let me know what you prefer and if I missed something.

pinpox commented 4 months ago

The nix flake archive approach has a drawback I wasn't aware of: It copies the flake itself plus all of it inputs from the deploying (local) machine to the remote. This can result in slow copies when deploying from a slow internet connection, as the cache and substituters don't seem to be used.

There is however a workaround: Using nix copy instead of nix flake archive copies only the flake itself, not the dependant inputs. During build everything that is missing will be tried to download form cache and substituters.

Both have negetatives and positives, the archive e.g. makes it possible to use inputs which are not publicly substitutable like private flakes or relative paths to other flakes.

I propose to make it configurable to the user whether to use nix flake archvive or nix copy and document the pros and cons

pinpox commented 4 months ago

I propose to make it configurable to the user whether to use nix flake archvive or nix copy and document the pros and cons

Implemented by: https://github.com/pinpox/lollypops/commit/ef001d67df7ca0418926fb3d05dbdcadaf29df01

GeoffreyFrogeye commented 4 months ago

I actually did as you described in #30 , before attempting to remove the symlink and config-dir. I made two separate commits so if you prefered keeping it we just had to remove/revert https://github.com/pinpox/lollypops/pull/33/commits/e399f87d1ed397d2f3c1dae1824bdd7883f8eb2e instead of manually undoing the changes 😅. I suppose I should have made that clearer? I always expect people to read commit descriptions and all but it's not super apparent on GitHub.

So yeah, I'm super fine with keeping the config dir and symlink, I only suggested to remove it to reduce complexity. Now I'm thinking I could do something with the directory (also keep a path to the build, so we could build first, and only deploy once ready) and flake (use it for auto-upgrades).

There's not much difference between my implementation and yours actually (I would put a nice link to see but I don't know how to do that with GitHub so here's a "link": git diff ccfd97c48f81109a2e719f868550b5af60ec1cd7 75485c01d3a42c03f4d6e30daadd19ce71ab316b). The only real thing is that as you suggested in #30 I used jq to filter the --json output and get the flake link, and you used {{.LOCAL_FLAKE_SOURCE}} directly. I think yours is cleaner, especially since I don't think we can implement a similar thing for nix flake copy.

If you agree, then I'd like to merge your changes into this branch, removing my second commit and thus keeping the history clean (also undoing the documentation changes in the process). Maybe just the changes before the nix flake copy of yours so you keep proper ownership of that.

pinpox commented 4 months ago

I actually did as you described in #30 , before attempting to remove the symlink and config-dir. I made two separate commits so if you prefered keeping it we just had to remove/revert e399f87 instead of manually undoing the changes 😅. I suppose I should have made that clearer?

Sorry, my bad, I should have read more carefully. Got a bit carried away testing/fixing.

I would put a nice link to see but I don't know how to do that with GitHub so here's a "link": git diff ccfd97c48f81109a2e719f868550b5af60ec1cd7 75485c01d3a42c03f4d6e30daadd19ce71ab316b

Here it is: https://github.com/pinpox/lollypops/compare/ccfd97c48f81109a2e719f868550b5af60ec1cd7...75485c01d3a42c03f4d6e30daadd19ce71ab316b

If you agree, then I'd like to merge your changes into this branch, removing my second commit and thus keeping the history clean (also undoing the documentation changes in the process). Maybe just the changes before the nix flake copy of yours so you keep proper ownership of that.

Yes, please do! Just merge the changes into yours. I'll keep the commit history so no worries about ownership, I mostly care about a clean implementation.

I have tested my branch with both copy methods and know they work, so it should be a good point to continue. Thank you again for taking the time to work on this!

GeoffreyFrogeye commented 4 months ago

Removed the commit, merged your changes in there, and fully tested (although I had to use https://github.com/pinpox/lollypops/pull/34/commits/17239c6339663e7e374cd18cf60b312176a17ed7 for testing local evaluation), all good for me!