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.

Sign up for GitHub

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

Jump to bottom

helm mapkubeapis plugin - also in offline packages #600

Merged
merged 1 commit into from
Jan 26, 2023
Merged

helm mapkubeapis plugin - also in offline packages #600

merged 1 commit into from
Jan 26, 2023

Conversation

jschaul
Copy link
Member

@jschaul jschaul commented Jan 25, 2023

supersven
supersven approved these changes Jan 26, 2023
View reviewed changes
Copy link
Contributor

@supersven supersven left a comment

Choose a reason for hiding this comment

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

Overall, LGTM 👍

I've left one comment. Solving it might make the code easier to digest.

nix/overlay.nix
@@ -1,4 +1,7 @@
self: super: {
self:
let helm-mapkubeapis = self.callPackage ./pkgs/helm-mapkubeapis.nix { };
Copy link
Contributor

Choose a reason for hiding this comment

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

I was a bit surprised seeing this expression here.

Have you tried to move it closer to its usage? E.g. to line 16?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did. But then it gave me "variable helm-mapkubeapis unknown" or something similar. When I moved it up here it worked.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not that bad. So, never mind 🤷 😄

@jschaul jschaul merged commit 8270456 into master Jan 26, 2023
@jschaul jschaul deleted the helm-mapkubeapis branch January 26, 2023 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@supersven supersven supersven approved these changes

@akshaymankar akshaymankar Awaiting requested review from akshaymankar

@julialongtin julialongtin Awaiting requested review from julialongtin julialongtin is a code owner

@sysvinit sysvinit Awaiting requested review from sysvinit

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jschaul @supersven