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

gsctl: init at 0.15.4 #63960

Merged
merged 2 commits into from Aug 17, 2019
Merged

gsctl: init at 0.15.4 #63960

merged 2 commits into from Aug 17, 2019

Conversation

JosephSalisbury
Copy link
Contributor

@JosephSalisbury JosephSalisbury commented Jun 30, 2019

Motivation for this change

We have a few folks at Giant Swarm using nix, so I reckoned I’d add it here :D

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@JosephSalisbury
Copy link
Contributor Author

@jagajaga @domenkozar mind having a look?

@domenkozar
Copy link
Member

@GrahamcOfBorg build


src = fetchFromGitHub {
owner = "giantswarm";
repo = "gsctl";
Copy link
Member

Choose a reason for hiding this comment

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

Can you use pname instead of name and use it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Look better?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, thank you!

@JosephSalisbury
Copy link
Contributor Author

Anything I can do to help with getting this merged?

Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

Hi @JosephSalisbury.

Typically, adding yourself to the maintainers list should be done in a separate commit like

maintainers: add joesalisbury

I've left some minor comments, but I can it does build and execute for me locally.

pkgs/applications/misc/gsctl/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/gsctl/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/gsctl/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/gsctl/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/gsctl/default.nix Outdated Show resolved Hide resolved
@JosephSalisbury JosephSalisbury changed the title gsctl: init at 0.15.1 gsctl: init at 0.15.4 Aug 8, 2019
@JosephSalisbury
Copy link
Contributor Author

Hey @worldofpeace, that should be all the comments addressed. Mind taking another pass?

@worldofpeace
Copy link
Contributor

@JosephSalisbury Why'd you drop adding yourself to the maintainers list?

Hope I didn't confuse you.

@JosephSalisbury
Copy link
Contributor Author

I possibly misinterpreted you - I was under the impression you wanted me to add it separately.

I can add it back here, or follow up with it. What would you recommend?

@worldofpeace
Copy link
Contributor

Your first commit in this PR should add yourself to maintainers-list.nix like

maintainers: add JosephSalisbury

and the second commit should be the package init and you should be listed as a maintainer in this package.

@JosephSalisbury
Copy link
Contributor Author

@worldofpeace hi! does that look better to you?

@worldofpeace
Copy link
Contributor

@worldofpeace hi! does that look better to you?

Yep 👍 Not sure why the bot added merge conflict. If there's one I can merge it manually.

@JosephSalisbury
Copy link
Contributor Author

There was one before I rebased, probably just a bit confused

@worldofpeace worldofpeace merged commit 85def1c into NixOS:master Aug 17, 2019
@worldofpeace
Copy link
Contributor

Thanks for contributing @JosephSalisbury.

@JosephSalisbury
Copy link
Contributor Author

Thanks for helping!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants