-
Notifications
You must be signed in to change notification settings - Fork 253
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
🌱 Import CAPI v1.6.0 #1731
🌱 Import CAPI v1.6.0 #1731
Conversation
✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
controller-runtime
to v0.16.2
controller-runtime
to v0.16.2
/assign mdbooth |
controller-runtime
to v0.16.2
controller-runtime
to v0.16.2
and deps
/hold |
controller-runtime
to v0.16.2
and deps
/test pull-cluster-api-provider-openstack-e2e-test |
I'm trying to bump Golang into a separate PR if possible, so we separate problems and fix them one by one: #1740 |
88b7516
to
b605574
Compare
This is working, cool. We are now waiting for the final CAPI 1.6.0 release (end of November), then I'll update the vendoring but the other commit should be an easy update. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Waiting with LGTM until the release is finalized
/test pull-cluster-api-provider-openstack-e2e-test |
004ae65
to
1a76ab7
Compare
@EmilienM FYI I bumped this to the release version. I'm not 100% sure about the removal of I see controller-runtime now supports secure metrics with kubernetes-sigs/controller-runtime#2407. I'm guessing we probably need to continue to support insecure metrics, though. In fact, would we need to add support for secure metrics by passing in a cert to use? |
config/manager/manager.yaml
Outdated
- containerPort: 8443 | ||
name: metrics | ||
protocol: TCP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hah, we were never exposing port 8080 anyway!
config/manager/manager.yaml
Outdated
- "--diagnostics-address=${CAPO_DIAGNOSTICS_ADDRESS:=:8443}" | ||
- "--insecure-diagnostics=${CAPO_INSECURE_DIAGNOSTICS:=false}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Until we've sorted out how we want to expose secure metrics, please can we put this back to the old, insecure. Also I'd prefer not to add new cluster api variables for substitution in this change. Lets do that separately and deliberately when we understand this feature.
For now we could either:
- Leave it as it is.
--metrics-bind-addr
is deprecated but still works and has the same behaviour as before. - Change it to
--diagnostics-address=localhost:8080 --insecure-diagnostics=true
, which keeps the same behaviour but without using the deprecated flag.
My feeling is that the prudent option is to entire revert the change to this file, leaving it exactly as it was before, and open an issue to track adding support for secure metrics.
Given that I think if we revert the change to I pre-emptively opened an issue to track adding it properly: #1777 |
Lets re-run the tests with the command line arguments change reverted... |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: EmilienM, mdbooth 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 |
/hold cancel |
/cherry-pick release-0.9 |
@mdbooth: once the present PR merges, I will cherry-pick it on top of release-0.9 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@mdbooth: new pull request created: #1780 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
What this PR does / why we need it:
Vendoring:
Code updates:
Fixes #1734
TODOs: