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.

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

✨Bump CAPI to v1.6.0-rc.1 #308

Merged
merged 2 commits into from
Dec 5, 2023
Merged

✨Bump CAPI to v1.6.0-rc.1 #308

merged 2 commits into from
Dec 5, 2023

Conversation

furkatgofurov7
Copy link
Member

@furkatgofurov7 furkatgofurov7 commented Oct 31, 2023 •
edited
Loading

What this PR does / why we need it:
Bumps CAPI to v1.6.0-rc.1 in main branch following https://main.cluster-api.sigs.k8s.io/developer/providers/migrations/v1.5-to-v1.6 migration guide

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 #299

/hold

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 31, 2023
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 31, 2023
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 1, 2023
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 2, 2023
@Danil-Grigorev
Copy link
Member

@furkatgofurov7 Can you please update it to the latest CAPI version?

@furkatgofurov7 furkatgofurov7 changed the title ✨Bump CAPI to v1.6.0-beta.0 ✨Bump CAPI to v1.6.0-rc.0 Nov 28, 2023
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 28, 2023
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 28, 2023
@furkatgofurov7
Copy link
Member Author

@Danil-Grigorev @alexander-demicev @Fedosin
I updated the PR to rc.0 (rc.1 should be out today, I'll probably bump to that version), however, it still needs some work in enabling secure diagnostics, I should be able to complete it by EOD and ping you here once it is ready

furkatgofurov7
furkatgofurov7 commented Nov 28, 2023
View reviewed changes
internal/envtest/environment.go
@@ -137,7 +145,7 @@ func New(uncachedObjs ...client.Object) *Environment {
}

// Set minNodeStartupTimeout for Test, so it does not need to be at least 30s
clusterv1.SetMinNodeStartupTimeout(metav1.Duration{Duration: 1 * time.Millisecond})
Copy link
Member Author

Choose a reason for hiding this comment

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

this is commented out now, because SetMinNodeStartupTimeout has been moved to internal package and I am not sure if we should have something similar in operator code or we can just drop it

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 28, 2023
@furkatgofurov7 furkatgofurov7 force-pushed the bump-beta-capi-1-6 branch 3 times, most recently from 2368beb to 407d091 Compare November 29, 2023 10:25
@furkatgofurov7
Copy link
Member Author

/test pull-cluster-api-operator-e2e-main

@furkatgofurov7 furkatgofurov7 force-pushed the bump-beta-capi-1-6 branch 6 times, most recently from 81d7bcc to 5412f2b Compare November 29, 2023 17:19
@furkatgofurov7 furkatgofurov7 changed the title ✨Bump CAPI to v1.6.0-rc.0 ✨Bump CAPI to v1.6.0-rc.1 Nov 29, 2023
@furkatgofurov7
Copy link
Member Author

/hold cancel

@alexander-demicev @Fedosin @Danil-Grigorev it should be ready for review 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 Nov 29, 2023
@Danil-Grigorev
Copy link
Member

/lgtm

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

LGTM label has been added.

Git tree hash: 3a79ea1dddad2f0ec784d8f4477e0495f76974d7

Fedosin
Fedosin reviewed Nov 29, 2023
View reviewed changes
internal/controller/component_customizer.go Outdated
@@ -190,10 +190,6 @@ func customizeManagerContainer(mSpec *operatorv1.ManagerSpec, c *corev1.Containe
c.Args = leaderElectionArgs(mSpec.LeaderElection, c.Args)
}

if mSpec.Metrics.BindAddress != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

This option is used by CAPI providers:

If we delete this, users won't be able to configure metrics anymore. Plus, it's a breaking change in our API, so we have to release v1alpha3 to accept this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Have you checked kubernetes-sigs/cluster-api#9264 upstream in CAPI? Both above providers are still using it, yes , but that is because they have not yet bumped to CAPI v1.6 AFAIK and 9264 suggests to remove --metrics-bind-addr and use --diagnostics-address=<addr> instead which should have same behaviour

Copy link
Contributor

Choose a reason for hiding this comment

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

We want to maintain support for installing older versions too... In other words, we want to allow CAPI operator to install CAPI providers with versions less than 1.6 without introducing breaking changes. With this change, if I have current versions of CAPA or CAPZ installed with modified metrics bind address, it works with CAPI operator v0.7.0, but won't work with v0.8.0.
IMO, we need to keep support of metrics-bind-addr (at least for several releases, before we introduce v1beta1), but also add diagnostics-address to our API. Potentially, we can implement a webhook conversion: metrics-bind-addr -> diagnostics-address.

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting, but then how about other providers removing it without any conversions (I saw a couple of them: CAPO, CAPZ)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we keep both and mark metrics-bind-addr as deprecated? I can see this is what is done in CAPI https://github.com/kubernetes-sigs/cluster-api/blob/main/util/flags/diagnostics.go#L45

Copy link
Member

Choose a reason for hiding this comment

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

As this is just a configuration value, it could be left in place, with the validation webhook preventing simultaneous use of both.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 30, 2023
alexander-demicev
alexander-demicev approved these changes Dec 5, 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.

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexander-demicev

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 Dec 5, 2023
Fedosin
Fedosin approved these changes Dec 5, 2023
View reviewed changes
Copy link
Contributor

@Fedosin Fedosin 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 Dec 5, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: d368ca0c0a8066c1b9f5f854352dbc5b8af5b9ea

@k8s-ci-robot k8s-ci-robot merged commit 55b2c9c into kubernetes-sigs:main Dec 5, 2023
10 checks passed
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

@Fedosin Fedosin Fedosin approved these changes

@JoelSpeed JoelSpeed Awaiting requested review from JoelSpeed

@Danil-Grigorev Danil-Grigorev Awaiting requested review from Danil-Grigorev

Assignees

@Fedosin Fedosin

@Danil-Grigorev Danil-Grigorev

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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CAPI v1.6.0-beta.0 has been released and is ready for testing
5 participants
@furkatgofurov7 @Danil-Grigorev @k8s-ci-robot @Fedosin @alexander-demicev