-
Notifications
You must be signed in to change notification settings - Fork 283
✨ Add PriorityQueue feature flag #2823
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
Conversation
✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hi @nikParasyr. Thanks for your PR. I'm waiting for a github.com 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-sigs/prow repository. |
| - "--v=2" | ||
| - "--diagnostics-address=127.0.0.1:8080" | ||
| - "--insecure-diagnostics=true" | ||
| - "--feature-gates=PriorityQueue=${EXP_CAPO_PRIORITY_QUEUE:=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.
Some notes about this:
- This will work fine when capo is installed through clusterctl (as they use envsub) but if someone uses the release manifests to directly install from it wont work for them
- I opted for
EXP_CAPO_PRIORITY_QUEUEto differentiate from capiEXP_PRIORITY_QUEUE, not sure if this is needed
ebf0c56 to
b541318
Compare
|
/ok-to-test |
bnallapeta
left a comment
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.
/lgtm
lentzi90
left a comment
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.
Code looks good. A couple of comments on the docs.
Add support for controller-runtime PriorityQueue. To do this: - introduce feature flags on the provider - create new feature flag that when set enables the controller-runtime PriorityQueue - document feature flags and how to enable them - document the PriorityQueue feature flag with upstream links
b541318 to
2d8d6f2
Compare
lentzi90
left a comment
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.
Thank you for this!
/approve
/hold cancel
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bnallapeta, lentzi90 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 |
|
I think this misses the /lgtm label |
|
Yes, lgtm is always removed when there are changes. |
|
/lgtm |
What this PR does / why we need it:
Add support for controller-runtime PriorityQueue.
To do this:
Which issue(s) this PR fixes:
Fixes #2820
Special notes for your reviewer:
a. keeping it similar with other providers
b. makes it easier to remove at a later stage as based on upstream
priorityQueuewill become defaultTODOs:
/hold