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): bump k8s to 1.24 #964

Conversation

everettraven
Copy link
Contributor

Description of the change:
Made changes to bump k8s dependencies to support k8s 1.24.

Changes:

  • bump dependencies in go.mod to support k8s 1.24
  • changes in vendor directory from running go mod vendor
  • update testdata CRDs spec to have the preserveUnknownFields: false value.
  • modifications to test cases in validate_test.go and bundle_test.go
    • There was some test failures happening that expected a warning like CRD etcdclusters.etcd.database.coreos.com/v1beta2 is present in bundle "etcdoperator.v0.9.4" but not defined in CSV to show up as an error. I found that the following shows that it should be a warning and not an error:
      // CRDs not defined in the CSV present in the bundle
      for key := range keySet {
      result.Add(errors.WarnInvalidBundle(fmt.Sprintf("CRD %s is present in bundle %q but not defined in CSV", keyToString(key), bundle.Name), key))
      }
      If this is not the case and it should be showing up as an error, I am happy to try and investigate this a bit further but would likely need some direction as to where that warning should be converted to an error instead of a warning.

Motivation for the change:
Begin supporting k8s 1.24

Reviewer Checklist

  • Implementation matches the proposed design, or proposal is updated to match implementation
  • Sufficient unit test coverage
  • Sufficient end-to-end test coverage
  • Docs updated or added to /docs
  • Commit messages sensible and descriptive

@codecov
Copy link

codecov bot commented May 25, 2022

Codecov Report

Merging #964 (3ab0a59) into master (cefb2f6) will decrease coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 3ab0a59 differs from pull request most recent head 02ee178. Consider uploading reports for the commit 02ee178 to get more accurate results

@@            Coverage Diff             @@
##           master     #964      +/-   ##
==========================================
- Coverage   52.48%   52.47%   -0.02%     
==========================================
  Files         103      103              
  Lines        9240     9240              
==========================================
- Hits         4850     4849       -1     
- Misses       3468     3469       +1     
  Partials      922      922              
Impacted Files Coverage Δ
pkg/registry/query.go 60.41% <0.00%> (-0.53%) ⬇️

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 cefb2f6...02ee178. Read the comment docs.

@joelanford
Copy link
Member

How hard would it be to bump the CRD versions from v1beta1 to v1? v1beta1 CRDs are no longer supported (since k8s 1.22), so it seems like those should be bumped at some point.

It seems like this could be done in a follow-up since the tests seem to have all passed.

@everettraven
Copy link
Contributor Author

everettraven commented May 25, 2022

How hard would it be to bump the CRD versions from v1beta1 to v1? v1beta1 CRDs are no longer supported (since k8s 1.22), so it seems like those should be bumped at some point.

It seems like this could be done in a follow-up since the tests seem to have all passed.

@joelanford I don't think it should be to difficult to do that, but can't say for sure. I agree that this would be a good follow up item. May even be able to get away with an update from v1beta1 --> v1alpha1 if it seems like that would be better than a v1beta1 --> v1 upgrade

Copy link
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

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

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 27, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dinhxuanvu, everettraven

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 27, 2022
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 31, 2022
@dinhxuanvu
Copy link
Member

@everettraven Would you mind rebasing this PR?

@everettraven
Copy link
Contributor Author

everettraven commented May 31, 2022

Would you mind rebasing this PR?

@dinhxuanvu just gave it a shot (still learning how to rebase). Let me know if it needs anything else.

Also noticed the e2e test failed while trying to install podman.

the go-apidiff is also an expected failure due to some changes in the ObjectMeta api in k8s 1.24

@dinhxuanvu
Copy link
Member

Would you mind rebasing this PR?

@dinhxuanvu just gave it a shot (still learning how to rebase). Let me know if it needs anything else.

Also noticed the e2e test failed while trying to install podman.

the go-apidiff is also an expected failure due to some changes in the ObjectMeta api in k8s 1.24

I'm trying to look into this podman installation problem. go-apidiff is expected to fail here so no worries about that.

@dinhxuanvu
Copy link
Member

The podman installation issue seems to be a known issue that is pending: containers/podman#14278 (comment)
We may need to wait for a bit to see if things are sorted out. Stay tuned.

@lsm5
Copy link

lsm5 commented May 31, 2022

3.4.2-5 has just been built and should now be available. Please give it a try. Thanks.

@everettraven
Copy link
Contributor Author

@dinhxuanvu is there anything else that needs to be done to get this merged?

@dinhxuanvu
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 1, 2022
@openshift-merge-robot openshift-merge-robot merged commit 043db4f into operator-framework:master Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants