Skip to content
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

Nix flake enablement for gardenctl-v2 #366

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

vasu1124
Copy link
Member

What this PR does / why we need it:

We want to enable gardenctl for the nix community.

# with Nix (macOS, Linux, and Windows)
nix run github:gardener/gardenctl-v2 -- version

# development version
nix profile install github:gardener/gardenctl-v2

gardenctl --help

@vasu1124 vasu1124 requested a review from a team as a code owner January 10, 2024 21:42
@gardener-robot gardener-robot added needs/review Needs review size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels Jan 10, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jan 10, 2024
Copy link

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left the explanations for the other things in open-component-model/ocm#625 (review) since I didn't want to copy paste them here.

flake.nix Outdated Show resolved Hide resolved
flake.nix Outdated Show resolved Hide resolved
flake.nix Outdated Show resolved Hide resolved
flake.nix Outdated Show resolved Hide resolved
flake.nix Outdated Show resolved Hide resolved
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jan 12, 2024
@Kumm-Kai
Copy link

Kumm-Kai commented Mar 11, 2024

Hi, great to see a flake for gardenctl ❤️

Would it be possible to also add an overlay to the flake?:

overlays.default = final: prev: {
  ${pname} = self.packages.${prev.system}.${pname};
};

This would allow users to declaratively install this flake using overrides (e.g., for nixpkgs), without needing to define this override function themselves.

As gardenctl heavily depends on gardenlogin it might be a good idea to add gardenlogin to the inputs of this flake and wrap the gardenlogin binary with gardenctl using wrapProgram in postFixup?
However, as I'm quite new to nix, I don't know if this is actually a good idea 😅

When trying out this flake I additionally encountered the following error on shell startup:
/dev/fd/16:22: permission denied: /Users/REDACTED/.nix-profile/share/zsh/site-functions/_gardenctl
This is triggered by including source <(gardenctl rc zsh) in my .zshrc. The sourced script tries to generate the completion script & write it to $GCTL_COMPLETION_FILE (/Users/REDACTED/.nix-profile/share/zsh/site-functions/_gardenctl)...

@vasu1124
Copy link
Member Author

Would it be possible to also add an overlay to the flake?

Give me some time to check the pro & cons. I first need the maintainers to merge this PR (but they insist that the vendorhash has to be automated as well ...)

add gardenlogin to the inputs of this flake

Will check as well. I currently like that this dependency is not made implicit (meaning, the user has to explicitly flake download each tool).

When trying out this flake I additionally encountered the following error on shell startup:

If you setup nix properly, the installShellCompletion of the derivation automagically includes the source <(tool-of-choice completion $SHELL), and those dotfiles/directories are made read-only. I think you have to seek out how to run your environment side by side with nix, the issue is that you are trying to overwrite nix.

@petersutter
Copy link
Contributor

I first need the maintainers to merge this PR (but they insist that the vendorhash has to be automated as well ...)

Out of good reason. Take https://github.com/open-component-model/ocm as an example. The PR Make ocm flake.nix ready #625 was merged, and the go.mod dependencies were updated several times afterwards. However, the vendorHash was not updated at all, which was foreseeable.

@Kumm-Kai
Copy link

I 100% agree that the vendorHash should be updated automatically (at the very least CI should check if the flake can still build)... Otherwise, it's pretty much useless 🙃

@vasu1124

If you setup nix properly, the installShellCompletion of the derivation automagically includes the source <(tool-of-choice completion $SHELL), and those dotfiles/directories are made read-only. I think you have to seek out how to run your environment side by side with nix, the issue is that you are trying to overwrite nix.

I'm aware that most tools (installed with nix) are "installing" the completion scripts into a location I can use by adding it to $fpath (which I already do). However, the source <(gardenctl rc zsh) call in my .zshrc (documented here) is additionally used to create aliases and "start" the gardenctl environment.

I could specify everything on my own, instead of using source <(gardenctl rc zsh), but IMO the script ideally shouldn't create & save the completion scripts on every shell startup / call of the script.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/review Needs review size/m Size of pull request is medium (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants