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

crossplane-cli: init at 1.14.3 #265354

Merged
merged 2 commits into from
Dec 1, 2023
Merged

crossplane-cli: init at 1.14.3 #265354

merged 2 commits into from
Dec 1, 2023

Conversation

selfuryon
Copy link
Contributor

Description of changes

Crossplane released a crossplane cli tool with 1.14.0 release version.

Nixpkgs already has a crossplane tool, so I decided to name it crossplane-cli.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@selfuryon selfuryon force-pushed the feat/crossplane-cli branch 2 times, most recently from 7138bdd to 0e6d582 Compare November 4, 2023 00:14
eclairevoyant
eclairevoyant suggested changes Nov 4, 2023
View reviewed changes
Copy link
Contributor

@eclairevoyant eclairevoyant left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Some feedback below. Additionally, please split the maintainer list change into a separate commit (should go first)

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
pkgs/applications/networking/cluster/crossplane-cli/default.nix Outdated Show resolved Hide resolved
IogaMaster
IogaMaster suggested changes Nov 5, 2023
View reviewed changes
Copy link
Contributor

@IogaMaster IogaMaster left a comment

Choose a reason for hiding this comment

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

Other than that looks great!
Welcome to nixpkgs and thank you for contributing! 😄

pkgs/by-name/cr/crossplane-cli/package.nix Outdated Show resolved Hide resolved
IogaMaster
IogaMaster approved these changes Nov 6, 2023
View reviewed changes
Copy link
Contributor

@IogaMaster IogaMaster left a comment

Choose a reason for hiding this comment

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

Looks great!

@selfuryon selfuryon mentioned this pull request Nov 15, 2023
Merged
13 tasks
@selfuryon
Copy link
Contributor Author

Do I need to do something for merging that? Ask somebody or do some additional actions?

@WolfangAukang
Copy link
Contributor

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

1 package built:
  • crossplane-cli

@WolfangAukang
Copy link
Contributor

Do I need to do something for merging that? Ask somebody or do some additional actions?

The "best" option is to send your PR to this thread. Sometimes it gets merged, sometimes it gets ignored (unfortunately)

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/1266

@mjgallag
Copy link

mjgallag commented Nov 29, 2023 •
edited
Loading

@selfuryon I don't think it's necessary to change the name of the binary too, as an example see kubernetes-helm which still uses the binary name helm.

@selfuryon selfuryon force-pushed the feat/crossplane-cli branch 2 times, most recently from 87d0f45 to 6673adf Compare November 29, 2023 23:58
@selfuryon
Copy link
Contributor Author

@selfuryon I don't think it's necessary to change the name of the binary too, as an example see kubernetes-helm which still uses the binary name helm.

It's the problem of naming:

  • Crossplane already has crossplane application: it's original crossplane controller in Kubernetes pod
  • Crossplane internally called this cli utility as crank, but in all manuals reference it like crossplane or crossplane cli. For example, their installation script rename crank to crossplane
  • nixpkgs already has crossplane app, so I needed to specify crossplane somehow, so I decided to name it crossplane-cli.

But how do you suggest to name it? crossplane-crank?

@selfuryon selfuryon changed the title crossplane-cli: init at 1.14.0 crossplane-cli: init at 1.14.3 Nov 30, 2023
mjgallag
mjgallag approved these changes Nov 30, 2023
View reviewed changes
Copy link

@mjgallag mjgallag left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@selfuryon I cannot merge as I'm not a maintainer but thank you for contributing this! I can confirm it works on my x86_64-darwin machine.

@emilytrau emilytrau merged commit f543200 into NixOS:master Dec 1, 2023
24 checks passed
@emilytrau
Copy link
Member

Thanks! Welcome to nixpkgs :)

@selfuryon selfuryon deleted the feat/crossplane-cli branch December 1, 2023 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjgallag mjgallag mjgallag approved these changes

@AndersonTorres AndersonTorres AndersonTorres approved these changes

@WolfangAukang WolfangAukang WolfangAukang approved these changes

@IogaMaster IogaMaster IogaMaster approved these changes

@eclairevoyant eclairevoyant Awaiting requested review from eclairevoyant

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer 12.approvals: 3+ 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@selfuryon @WolfangAukang @nixos-discourse @mjgallag @emilytrau @eclairevoyant @AndersonTorres @IogaMaster @delroth @NixOSInfra