-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Add ProvReq annotations to the api package #6759
Add ProvReq annotations to the api package #6759
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: yaroslava-serdiuk The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
07b0e7c
to
3c3e12d
Compare
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 doing this!
I guess that we should remove existing annotations
autoscaler/cluster-autoscaler/provisioningrequest/pods/pods.go
Lines 30 to 35 in 0c26da3
const ( | |
// ProvisioningRequestPodAnnotationKey is a key used to annotate pods consuming provisioning request. | |
ProvisioningRequestPodAnnotationKey = "cluster-autoscaler.kubernetes.io/consume-provisioning-request" | |
// ProvisioningClassPodAnnotationKey is a key used to add annotation about Provisioning Class | |
ProvisioningClassPodAnnotationKey = "cluster-autoscaler.kubernetes.io/provisioning-class-name" | |
) |
3c3e12d
to
c246393
Compare
|
||
const ( | ||
// ProvisioningRequestPodAnnotationKey is a key used to annotate pods consuming provisioning request. | ||
ProvisioningRequestPodAnnotationKey = "cluster-autoscaler.kubernetes.io/consume-provisioning-request" |
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.
Hmm, @MaciekPytel we probably want x-k8s.io
instead of kubernetes.io
here as well, right? @yaroslava-serdiuk there aren't any pods in the wild using these annotations yet, right?
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.
Agreed, it is completely inconsistent with the rest of the names in this API. If there are any pods using it already, we may need to do a deprecation where we support both for a while, but we need to be consistent with those APIs.
If it's just a cluster autoscaler API, we should put cluster autoscaler in API group and everywhere else. If it's kubernetes API, than it shouldn't use annotation that specifically refers to CA
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.
Do you mean we need to have "autoscaling.x-k8s.io/consume-provisioning-request"?
Assigning since looks like you're reviewing this one already. /assign @towca |
PR needs rebase. 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. |
@yaroslava-serdiuk What is the reason that this PR was closed? Does this mean that these annotations are never exposed via API module? |
@tenzen-y My understanding is that the API module should include annotations with |
What type of PR is this?
/kind cleanup