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

eksctl: init at 0.1.35 #63041

Merged
merged 1 commit into from
Jun 15, 2019
Merged

eksctl: init at 0.1.35 #63041

merged 1 commit into from
Jun 15, 2019
+42 −0

Conversation

xrelkd
Copy link
Contributor

@xrelkd xrelkd commented Jun 12, 2019

Motivation for this change

eksctl is a CLI for Amazon EKS
https://eksctl.io
https://github.com/weaveworks/eksctl

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.

Sorry, something went wrong.

JohnAZoidberg
JohnAZoidberg reviewed Jun 12, 2019
View reviewed changes
pkgs/tools/admin/eksctl/default.nix Outdated

meta = with stdenv.lib; {
description = "A CLI for Amazon EKS";
homepage = https://github.com/weaveworks/eksctl;
Copy link
Member

Choose a reason for hiding this comment

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

Hint: We're probably going to deprecate unquotes URL syntax when RFC 45 is approved.

Suggested change
homepage = https://github.com/weaveworks/eksctl;
homepage = "https://github.com/weaveworks/eksctl";

pkgs/tools/admin/eksctl/default.nix Outdated

goPackagePath = "github.com/weaveworks/eksctl";

buildPhase =
Copy link
Member

Choose a reason for hiding this comment

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

Do you need the custom buildPhase?
I left it out and it builds fine - I have no AWS however, so I can't test.

pkgs/tools/admin/eksctl/default.nix Outdated
@@ -0,0 +1,40 @@
{ stdenv, buildGoPackage, fetchFromGitHub }:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{ stdenv, buildGoPackage, fetchFromGitHub }:
{ lib, buildGoPackage, fetchFromGitHub }:

If you're only using stdenv.lib you might as well just use lib directly here.

@xrelkd
Copy link
Contributor Author

xrelkd commented Jun 13, 2019

@JohnAZoidberg Thanks for the review!
I need the custom buildPhase, because

  1. I want to add netgo and release tag.
  2. We do not need to build result-bin/bin/build

@JohnAZoidberg
Copy link
Member

I think if we replace the buildPhase with

  subPackages = [ "cmd/eksctl" ];

  buildFlags = ''
    -ldflags=-c
    -ldflags=-w
    -tags netgo
    -tags release
  '';

it does exactly what you want.
I just couldn't get the ldflags and tags to combine as one argument each.

@xrelkd
Copy link
Contributor Author

xrelkd commented Jun 13, 2019

@JohnAZoidberg Thank you!
I have updated the code as your suggestions.

@JohnAZoidberg
Copy link
Member

JohnAZoidberg commented Jun 13, 2019 •
edited
Loading

What do the linkerflags do and why do you include them?

@xrelkd
eksctl: init at 0.1.35

Unverified

The committer email address is not verified.
Learn about vigilant mode
66ee3ae
@xrelkd
Copy link
Contributor Author

xrelkd commented Jun 13, 2019

The -w turns off DWARF debugging information.
The -s turns off generation of the Go symbol table.

Take a look at this file: https://github.com/weaveworks/eksctl/blob/master/.goreleaser.yml

@xrelkd
Copy link
Contributor Author

xrelkd commented Jun 15, 2019

@JohnAZoidberg Thanks for the review.
@marsam Could we merge this PR? Thank you!

@JohnAZoidberg
Copy link
Member

I haven't tried whether it works and does the right thing. I trust that you have?

@xrelkd
Copy link
Contributor Author

xrelkd commented Jun 15, 2019

@JohnAZoidberg
Yes, I have tried to use eksctl to create a AWS EKS cluster and delete the created AWS EKS cluster after that.
eksctl works as expected.

BTW, could you invoke @GrahamcOfBorg build eksctl, if possible?
Thank you!

@marsam
Copy link
Contributor

marsam commented Jun 15, 2019

LGTM, Thanks!
@GrahamcOfBorg build eksctl

@marsam marsam merged commit 1cafe46 into NixOS:master Jun 15, 2019
@xrelkd
Copy link
Contributor Author

xrelkd commented Jun 15, 2019

@JohnAZoidberg @marsam Thank you very much!

@xrelkd xrelkd deleted the add/eksctl branch June 15, 2019 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JohnAZoidberg JohnAZoidberg

Assignees
No one assigned
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@xrelkd @JohnAZoidberg @marsam