-
-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
kubernetes-kcp: init at 0.26.0 #357444
base: master
Are you sure you want to change the base?
kubernetes-kcp: init at 0.26.0 #357444
Conversation
2c73ded
to
c2d577f
Compare
I'm not sure what the next step should be -- do I need to tag someone who has write access to merge the PR? |
Yes, someone with the commit bit will pass by eventually. You could look for someone who often merges go modules and request a review By the way, it probably makes sense to
|
# "-X k8s.io/client-go/pkg/version.gitMajor=${KUBE_MAJOR_VERSION}" | ||
# "-X k8s.io/client-go/pkg/version.gitMinor=${KUBE_MINOR_VERSION}" | ||
"-X k8s.io/client-go/pkg/version.buildDate=unknown" | ||
"-X k8s.io/component-base/version.gitCommit=${rev}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to, just mark unkown
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Aleksanaa Could you elaborate why that's the case? Is this just customary, and/or because it's easier to update the reference for updates by simply using the tag?
This will be a slight diversion from the original binary, and when it is as simple as providing a commit reference, I thought it's worth keeping it around. (Also we should technically have the Kubernetes major / minor version reference, which would ensure the Nix package will be the same as the official binary.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because setting rev specifically will make automatic update wrong, and not very useful as the version is sufficient to find the revision
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification! The automatic update support would be nice to have, and indeed the tag should give us the revision. But it is also possible that the tag gets removed and recreated for a different commit (though unlikely), and this revision reference acts as a safety net in a way I suppose.
In either case, it is also not a fast moving project AFAIAA, and it is something I can revisit later. The change has been made to remove the unnecessary let binding and git commit reference kept as unknown.
Thanks for the pointer, @steeleduncan -- regarding your points:
I kept the TODO items as those are something we should handle for proper feature parity (it may be a stretch to call this a feature, but nonetheless a difference from the official binary). |
@Aleksanaa Thanks for the review! I made the suggested changes and a few adjustments (namely argument for I also went ahead to squash the code change commits into a single commit. My understanding is that I will need to keep the contributor addition commit separate, so that was kept as is. Rebase / squash / force-push made it difficult to see the diff, but I hope this one is simple enough to wrap your head around the change 🙏 |
19bd210
to
2d47f53
Compare
Update pkgs/applications/networking/cluster/kcp/default.nix Co-authored-by: Aleksana <[email protected]> Update pkgs/applications/networking/cluster/kcp/default.nix Co-authored-by: Aleksana <[email protected]> Update pkgs/applications/networking/cluster/kcp/default.nix Co-authored-by: Aleksana <[email protected]> Move package to pkgs/by-name/ku/kubernetes-kcp Fix format error
Introduces kcp, Kubernetes-like control planes for form-factors and use-cases beyond Kubernetes and container workloads.
Closes #357434
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.