-
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
✨ Backport Provider to release-0.7 #1529
✨ Backport Provider to release-0.7 #1529
Conversation
|
✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
Welcome @Jeremy-Boyle! |
Hi @Jeremy-Boyle. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
/ok-to-test |
usually we don't want to backport features to older release, mostly we backport bugs.. @mdbooth @seanschneeweiss any thoughts? |
@jichenjc There's a discussion on slack about backporting this feature to release 0.7 |
Thanks for sharing , didn't notice that :( |
I'd normally agree, but:
So we thought this was a pragmatic way forward. |
/lgtm I'll approve this after #1530 lands in main. |
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 good! I pointed out something I noticed inline, but this is part of the original patch, so obviously no need to fix it here.
record.Warnf(openStackCluster, "SkippedCreateMonitor", "Health monitors are not supported with the current Octavia provider.") | ||
record.Eventf(openStackCluster, "SkippedCreateMonitor", "Health Monitor is not created as it's not implemented with the current Octavia provider.") |
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.
Only now I realized both Warnf()
and Eventf()
produce an event. We shouldn't need both, just Warnf()
should be enough. Obviously that isn't something to fix here, but rather on master
branch.
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.
Good catch, this is a carbon copy from what was inside of main from the existing v1alpha7 was trying to keep it as close as possible
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.
Yup, it should be kept exactly like this. We can fix this on master and then backport the fix.
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.
@dulek Do you want to submit that while it's fresh in your mind?
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.
is now in main |
/hold cancel |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dulek, Jeremy-Boyle, 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 |
What this PR does / why we need it:
This back ports the Provider feature into
v1alpha6
, numerous users would like the feature howevermain
is not in a state to push a release.Back port is based on PR: #1501
Special notes for your reviewer:
TODOs:
v1alpha6
PR: 🐛 Patch: Backport Provider to v1alpha6 #1530/hold