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

chore(deps): Bump sigs.k8s.io/controller-tools/cmd/controller-gen from v0.4.1 to v0.14.0 #12719

Merged
merged 6 commits into from
Mar 11, 2024

Conversation

blkperl
Copy link
Contributor

@blkperl blkperl commented Feb 29, 2024

Motivation

This PR upgrades controller-gen to a modern version of controller-gen. The prior version was released in 2020. Some of the manipulation in https://github.com/argoproj/argo-workflows/blob/main/hack/crds.go may no longer be required but it doesn't change the final result either way.

This is part of a series of PRs to update build dependencies until the build no longer panics on Golang 1.22

Modifications

The new version of controller-gen adds additional x-kubernetes-map-type which is described in kubernetes/kubernetes#84113. The map-type is manipulated in https://github.com/argoproj/argo-workflows/blob/main/hack/crds.go#L65 and was added in #6350. This manipulation may no longer be required.

Verification

I didn't do any manual testing as any issues would be caught by e2e tests.

…m v0.4.1 to v0.14.0

Signed-off-by: william.vanhevelingen <[email protected]>
@blkperl blkperl marked this pull request as ready for review February 29, 2024 21:20
@blkperl blkperl added type/dependencies PRs and issues specific to updating dependencies go Pull requests that update Go dependencies labels Feb 29, 2024
Signed-off-by: William Van Hevelingen <[email protected]>
@@ -255,7 +255,7 @@

controller-tools = pkgs.buildGoModule rec {
pname = "controller-tools";
version = "0.4.1"; # upgrade this in the Makefile if upgraded here
version = "0.14.0"; # upgrade this in the Makefile if upgraded 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.

Anyone know why it built without having to change sha256 or vendorHash?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question; this is beyond my knowledge of Nix, cc @isubasinghe

Copy link
Member

Choose a reason for hiding this comment

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

Did you run nix develop, this package isn't part of the build only the shell.

error: hash mismatch in fixed-output derivation '/nix/store/ajavbdx0j71cp0fp509z84anmgmc29mv-source.drv':
         specified: sha256-NQlSP9hRLXr+iZo0OeyF1MQs3PourQZN0I0v4Wv5dkE=
            got:    sha256-G0jBQ12cpjfWGhXYppV9dB2n68bExi6ME9QbxXsUWvw=

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any idea why it doesn't throw the error for me?

(devenv) @blkperl ➜ ~/go/src/github.com/argoproj/argo-workflows (controller-gen) $ nix develop --extra-experimental-features nix-command --extra-experimental-features flakes ./dev/nix/ 
wait-for minio
deployment.apps/minio condition met
port-forward minio 9000
Forwarding from 127.0.0.1:9000 -> 9000
Forwarding from [::1]:9000 -> 9000
port-forward minio 9001
Forwarding from 127.0.0.1:9001 -> 9001
Forwarding from [::1]:9001 -> 9001
yarn install v1.22.19
[1/5] Validating package.json...
[2/5] Resolving packages...
success Already up-to-date.
Done in 0.54s.
(devenv) @blkperl ➜ ~/go/src/github.com/argoproj/argo-workflows (controller-gen) $ 

Copy link
Member

Choose a reason for hiding this comment

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

Weird, could it be a a cache thing perhaps?

Copy link
Contributor

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

Could you fill out the PR template? That would be helpful in describing the rationale, as well as explaining why all the lines got added in the CRDs and why one of the options was removed. E.g. linking to the relevant docs or changelogs for posterity

@@ -91,6 +91,7 @@ spec:
required:
- key
type: object
x-kubernetes-map-type: atomic
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like all of the >1000 LoC added to the CRDs is this same line

Copy link
Contributor Author

@blkperl blkperl Mar 1, 2024

Choose a reason for hiding this comment

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

It's a feature of modern versions of K8s and is described here: kubernetes/kubernetes#84113 .

It is manipulated here but there is no comments describing why.

https://github.com/argoproj/argo-workflows/blob/main/hack/crds.go#L65

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like it might still be necessary per #6350's logic. x-kubernetes-map-type: atomic was already used, but this dep change ended up adding it to more places -- why more were added due to this version of controller-gen is not entirely clear.

@@ -255,7 +255,7 @@

controller-tools = pkgs.buildGoModule rec {
pname = "controller-tools";
version = "0.4.1"; # upgrade this in the Makefile if upgraded here
version = "0.14.0"; # upgrade this in the Makefile if upgraded here
Copy link
Contributor

Choose a reason for hiding this comment

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

Good question; this is beyond my knowledge of Nix, cc @isubasinghe

@agilgur5 agilgur5 added the area/build Build or GithubAction/CI issues label Mar 1, 2024
Copy link
Member

@isubasinghe isubasinghe left a comment

Choose a reason for hiding this comment

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

I've specified the correct hash in the comment, should be a quick fix.

@blkperl
Copy link
Contributor Author

blkperl commented Mar 8, 2024

@isubasinghe I've added the suggested SHA.

@isubasinghe
Copy link
Member

Note: doesn't build under Nix anyway.

Will fix this myself.

@blkperl feel free to make changes and just ping me that you upgraded a dependency next time, I will take responsibility for the Nix until its a bit more automated (in terms of hash fixing).

@isubasinghe isubasinghe enabled auto-merge (squash) March 11, 2024 11:34
@isubasinghe isubasinghe merged commit 0270a0f into argoproj:main Mar 11, 2024
27 checks passed
@blkperl blkperl deleted the controller-gen branch March 13, 2024 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build Build or GithubAction/CI issues go Pull requests that update Go dependencies type/dependencies PRs and issues specific to updating dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants