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

copilot-cli: init at 0.6.0 #103255

Closed
wants to merge 2 commits into from
Closed

copilot-cli: init at 0.6.0 #103255

wants to merge 2 commits into from

Conversation

dbirks
Copy link
Member

@dbirks dbirks commented Nov 10, 2020

Motivation for this change

Wanted to add Amazon's new copilot-cli. Set the version to the most recent that was released today.

The ldflags are long, but I believe binaryS3BucketPath is needed based on the Makefile. Looks like it's used in the buildspec template file, to be used when setting up a pipeline.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

subPackages = [ "cmd/copilot" ];

buildFlagsArray = [
"-ldflags=-s -w -X github.com/aws/copilot-cli/internal/pkg/version.Version=${src.rev} -X github.com/aws/copilot-cli/internal/pkg/cli.binaryS3BucketPath=https://ecs-cli-v2-release.s3.amazonaws.com"
Copy link
Member

Choose a reason for hiding this comment

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

I'm worried about this binaryS3BucketPath, if it's changed upstream we may not notice that in a regular update. Can you try to make this work using make instead? See #103037 for inspiration on how to do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you, that's a good idea. It ends up being a lot cleaner too. Just switched over to it.

@kalbasit kalbasit marked this pull request as draft November 10, 2020 17:55
@dbirks dbirks changed the title copilot: init at 0.6.0 copilot-cli: init at 0.6.0 Nov 11, 2020
@dbirks dbirks marked this pull request as ready for review November 11, 2020 03:02
@dbirks dbirks requested a review from kalbasit November 11, 2020 03:02
Copy link
Member

@IvarWithoutBones IvarWithoutBones left a comment

Choose a reason for hiding this comment

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

Just 1 nit :)

Comment on lines +16 to +18
buildPhase = ''
make VERSION=${src.rev} compile-local
'';
Copy link
Member

Choose a reason for hiding this comment

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

This could be done using makeFlags as well.

Suggested change
buildPhase = ''
make VERSION=${src.rev} compile-local
'';
makeFlags = [ "VERSION=${src.rev}" "compile-local" ];

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I tried this out, but I'm getting a strange error when building with nixpkgs-review:

error: pkgs/tools/admin/copilot-cli/default.nix: No such file or directory

This might not be related, but it got me thinking... is makeFlags maybe only part of mkDerivation, and not buildGoModule?

Copy link
Member

Choose a reason for hiding this comment

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

Please use preBuild to keep

Suggested change
buildPhase = ''
make VERSION=${src.rev} compile-local
'';
buildFlagsArray = [ "-ldflags=-s -w -X github.com/aws/copilot-cli/internal/pkg/version.Version=${version} -X github.com/aws/copilot-cli/internal/pkg/cli.binaryS3BucketPath=https://ecs-cli-v2-release.s3.amazonaws.com" ];

@SuperSandro2000
Copy link
Member

Result of nixpkgs-review pr 103255 run on x86_64-darwin 1

1 package built:
  • copilot-cli

@SuperSandro2000
Copy link
Member

Result of nixpkgs-review pr 103255 run on x86_64-linux 1

1 package built:
  • copilot-cli

Comment on lines +16 to +18
buildPhase = ''
make VERSION=${src.rev} compile-local
'';
Copy link
Member

Choose a reason for hiding this comment

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

Please use preBuild to keep

Suggested change
buildPhase = ''
make VERSION=${src.rev} compile-local
'';
buildFlagsArray = [ "-ldflags=-s -w -X github.com/aws/copilot-cli/internal/pkg/version.Version=${version} -X github.com/aws/copilot-cli/internal/pkg/cli.binaryS3BucketPath=https://ecs-cli-v2-release.s3.amazonaws.com" ];

@dbirks dbirks marked this pull request as draft December 29, 2020 05:39
@stale
Copy link

stale bot commented Jun 28, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 28, 2021
@jiegec jiegec mentioned this pull request Jul 10, 2022
13 tasks
@dbirks
Copy link
Member Author

dbirks commented Sep 16, 2022

🎉 Superseded by #180844

@dbirks dbirks closed this Sep 16, 2022
@dbirks dbirks deleted the add-copilot branch September 16, 2022 20:51
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

4 participants