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

Promote crd to apiextensions.k8s.io/v1 #1456

Merged
merged 11 commits into from
Jun 4, 2021

Conversation

rubenvp8510
Copy link
Collaborator

@rubenvp8510 rubenvp8510 commented May 19, 2021

Signed-off-by: Ruben Vargas [email protected]

This might break compatibility with k8s < 1.19, but is a necessary step to support k8s 1.22+

https://kubernetes.io/docs/reference/using-api/deprecation-guide/#v1-22

@rubenvp8510 rubenvp8510 force-pushed the crd-to-v1 branch 2 times, most recently from 2151eb0 to bf5f45e Compare May 19, 2021 05:42
@codecov
Copy link

codecov bot commented May 19, 2021

Codecov Report

Merging #1456 (f6347b3) into master (f7cee05) will increase coverage by 0.44%.
The diff coverage is 84.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1456      +/-   ##
==========================================
+ Coverage   86.88%   87.33%   +0.44%     
==========================================
  Files          91       90       -1     
  Lines        5071     4982      -89     
==========================================
- Hits         4406     4351      -55     
+ Misses        510      478      -32     
+ Partials      155      153       -2     
Impacted Files Coverage Δ
pkg/autodetect/main.go 86.13% <ø> (+4.49%) ⬆️
pkg/strategy/strategy.go 95.58% <ø> (ø)
pkg/controller/jaeger/ingress.go 74.19% <20.00%> (-0.81%) ⬇️
pkg/ingress/query.go 100.00% <100.00%> (ø)
pkg/inventory/ingress.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f7cee05...f6347b3. Read the comment docs.

Copy link
Contributor

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

LGTM, but just to confirm: did you try this in OpenShift as well?

@jpkrohling jpkrohling enabled auto-merge (squash) May 19, 2021 10:01
@@ -393,7 +393,7 @@ install-tools:
golang.org/x/lint/golint \
golang.org/x/tools/cmd/goimports \
github.com/securego/gosec/cmd/[email protected] \
sigs.k8s.io/controller-tools/cmd/controller-gen@v0.3.0 \
sigs.k8s.io/controller-tools/cmd/controller-gen@v0.5.0 \
Copy link
Member

Choose a reason for hiding this comment

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

Did bumping this version update the CRD yamls?

Copy link
Member

Choose a reason for hiding this comment

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

or did you run any command to generate CRD yalms?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I ran the controller-gen command and it generates the new CRD with the version.

@rubenvp8510
Copy link
Collaborator Author

LGTM, but just to confirm: did you try this in OpenShift as well?

Tested with CRC and all-in-one strategy and it works .

@jpkrohling
Copy link
Contributor

I think I restarted the tests a couple of times already. If this fails again, it might be a real failure, @rubenvp8510

@rubenvp8510
Copy link
Collaborator Author

I think I restarted the tests a couple of times already. If this fails again, it might be a real failure, @rubenvp8510

Yes, I was investigating about the failures, it seems like I need to migrate the Ingress API too, and because of that I need to bump some versions of the libraries, specifically k8s.io/api. I'll request your review again once I finish.

@rubenvp8510 rubenvp8510 force-pushed the crd-to-v1 branch 12 times, most recently from 6f98ffe to 9dcce55 Compare May 25, 2021 17:15
@rubenvp8510 rubenvp8510 disabled auto-merge May 25, 2021 20:53
@rubenvp8510 rubenvp8510 force-pushed the crd-to-v1 branch 2 times, most recently from 8e383b7 to 7545013 Compare May 25, 2021 21:19
Signed-off-by: Ruben Vargas <[email protected]>
Signed-off-by: Ruben Vargas <[email protected]>
Signed-off-by: Ruben Vargas <[email protected]>
@rubenvp8510 rubenvp8510 force-pushed the crd-to-v1 branch 6 times, most recently from 7268950 to e329846 Compare May 31, 2021 15:31
Signed-off-by: Ruben Vargas <[email protected]>
@rubenvp8510 rubenvp8510 force-pushed the crd-to-v1 branch 2 times, most recently from 913a2f3 to a5e39d0 Compare June 1, 2021 02:08
Signed-off-by: Ruben Vargas <[email protected]>
Copy link
Member

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

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

LGTM,

does this PR break compatibility with older k8s clusters?

- uses: actions/[email protected]
- name: "setup docker"
run: ./.ci/setup-docker.sh
- uses: manusa/[email protected]
Copy link
Member

Choose a reason for hiding this comment

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

nice I didn't see this. In the future we could also consider using kind.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, the new e2e tests are using kind and kuttl :-)

@rubenvp8510
Copy link
Collaborator Author

LGTM,

does this PR break compatibility with older k8s clusters?

Yes :( 1.19+ is required.

@jpkrohling jpkrohling enabled auto-merge (squash) June 4, 2021 08:12
@jpkrohling jpkrohling merged commit b601fc5 into jaegertracing:master Jun 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants