Skip to content

🌱 Bump Cluster API to v1.5.0-rc.0 #186

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
Merged
merged 1 commit into from
Jul 21, 2023
Merged

🌱 Bump Cluster API to v1.5.0-rc.0 #186

merged 1 commit into from
Jul 21, 2023

Conversation

Fedosin
Copy link
Contributor

@Fedosin Fedosin commented Jul 10, 2023

What this PR does / why we need it:
This PR update CAPI operator to the latest Cluster API version.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #183

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 10, 2023
@Fedosin
Copy link
Contributor Author

Fedosin commented Jul 10, 2023

/hold

waiting for the new version to be released.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 10, 2023
@Fedosin Fedosin force-pushed the capi15 branch 4 times, most recently from e05ea81 to 8b612ab Compare July 10, 2023 22:37
furkatgofurov7
furkatgofurov7 reviewed Jul 11, 2023
View reviewed changes
Copy link
Member

@furkatgofurov7 furkatgofurov7 left a comment

Choose a reason for hiding this comment

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

@Fedosin thanks for the PR, overall looks good to me, only one missing update I noticed based on the migration doc https://main.cluster-api.sigs.k8s.io/developer/providers/migrations/v1.4-to-v1.5.html#go-version and few more suggestions inline:

/retitle 🌱 Bump Cluster API to v1.5.0-beta.1

internal/controller/client_proxy.go
Comment on lines -103 to -109
{
GroupVersion: "cert-manager.io/v1",
APIResources: []metav1.APIResource{
{Kind: "Certificate", Namespaced: true},
{Kind: "Issuer", Namespaced: true},
},
},
Copy link
Member

Choose a reason for hiding this comment

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

why are we removing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are 2 reasons for that:

  1. In clusterctl they explicitly declare that this function should not return cert-manager objects. It's better to keep things consistent with cluster-api.
  2. After upgrading to controller-runtime v0.15.0 some tests fail because they need cert-manager CRDs installed (I still don't understand why it works in the previous versions). I don't want to add cert-manager to the operator dependencies, so I just decided to remove the section.

@k8s-ci-robot k8s-ci-robot changed the title 🌱 Bump Cluster API to v1.5 🌱 Bump Cluster API to v1.5.0-beta.1 Jul 11, 2023
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 12, 2023
@Fedosin
Copy link
Contributor Author

Fedosin commented Jul 12, 2023

I updated container images for e2e tests to make them work with golang 1.20: kubernetes/test-infra#30077

@furkatgofurov7 furkatgofurov7 mentioned this pull request Jul 13, 2023
Merged
furkatgofurov7
furkatgofurov7 reviewed Jul 13, 2023
View reviewed changes
@furkatgofurov7
Copy link
Member

/retest

@furkatgofurov7
Copy link
Member

furkatgofurov7 commented Jul 13, 2023 •
edited
Loading

waiting for the new version to be released.

We can always merge the rc bump and give the CI time to run longer with it to see if there are problems, so that whenever actual 1.5.0 comes out, it is just a matter of updating go.mod, nothing more.

@furkatgofurov7
Copy link
Member

/retitle 🌱 Bump Cluster API to v1.5.0-rc.0

@k8s-ci-robot k8s-ci-robot changed the title 🌱 Bump Cluster API to v1.5.0-beta.1 🌱 Bump Cluster API to v1.5.0-rc.0 Jul 13, 2023
@Fedosin
Copy link
Contributor Author

Fedosin commented Jul 13, 2023

@furkatgofurov7 I'm okay to merge #187 first and then I'll update my PR.

@furkatgofurov7
Copy link
Member

@furkatgofurov7 I'm okay to merge #187 first and then I'll update my PR.

@Fedosin sure, it should be ready to merge now, PTAL

@furkatgofurov7
Copy link
Member

@Fedosin #187 is merged, please rebase when you get a time, thanks!

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 13, 2023
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 14, 2023
furkatgofurov7
furkatgofurov7 approved these changes Jul 14, 2023
View reviewed changes
Copy link
Member

@furkatgofurov7 furkatgofurov7 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 working on this @Fedosin!

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: furkatgofurov7

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 14, 2023
alexander-demicev
alexander-demicev approved these changes Jul 21, 2023
View reviewed changes
Copy link
Contributor

@alexander-demicev alexander-demicev left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 21, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: d2fdc8b097eec310ad623578af7077072fa9544c

@furkatgofurov7
Copy link
Member

/hold cancel

We will bump to v1.5 CAPI once out (fairly straightforward then this one), this we can take in now.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 21, 2023
@k8s-ci-robot k8s-ci-robot merged commit 1b9007d into kubernetes-sigs:main Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexander-demicev alexander-demicev alexander-demicev approved these changes

@furkatgofurov7 furkatgofurov7 furkatgofurov7 approved these changes

@justinsb justinsb Awaiting requested review from justinsb

Assignees

@alexander-demicev alexander-demicev

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

CAPI v1.5.0-beta.0 has been released and is ready for testing

4 participants

@Fedosin @furkatgofurov7 @k8s-ci-robot @alexander-demicev