-
Notifications
You must be signed in to change notification settings - Fork 39k
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
Graduate Quota configuration API to v1beta1 #66156
Graduate Quota configuration API to v1beta1 #66156
Conversation
@vikaschoudhary16 - Can you please look at the lint failures: Do you want to modify e2e's to use beta version in a follow-up PR? |
3c1aec1
to
4ef4295
Compare
@ravisantoshgudimetla golint should be happy now.
There is no existing e2e test which is using this api. Also quoting @aveshagarwal
Though i am still thinking how to add an e2e for this. Will create a different PR, if i figured it out. Meanwhile would like to know suggestions if got any. |
4ef4295
to
e4c0680
Compare
19f96d2
to
2edb9d1
Compare
2edb9d1
to
bfaf8d2
Compare
/status approved-for-milestone |
[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process Pull Request Labels
|
LGTM overall :) |
@vikaschoudhary16 The way I have done moving api from alpha to beta in the past is ensuring alpha versionexists while creating beta. This ensures people using alpha have some breathing space before switching to beta. Please take a look at https://github.com/kubernetes/community/blob/master/contributors/devel/api_changes.md, while the wording is not clear on removing alpha version, it explicitly tells to copy https://github.com/kubernetes/community/blob/master/contributors/devel/api_changes.md#making-a-new-api-version |
@@ -16,7 +16,7 @@ go_library( | |||
"zz_generated.deepcopy.go", | |||
"zz_generated.defaults.go", | |||
], | |||
importpath = "k8s.io/kubernetes/plugin/pkg/admission/resourcequota/apis/resourcequota/v1alpha1", |
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.
Please do not delete (rename) v1alpha1 files. If we deleted these files, v1alpha1 APIs would stop working in the next K8s release.
bfaf8d2
to
afd71f6
Compare
@@ -28,7 +28,7 @@ go_library( | |||
"//pkg/util/workqueue/prometheus:go_default_library", | |||
"//plugin/pkg/admission/resourcequota/apis/resourcequota:go_default_library", | |||
"//plugin/pkg/admission/resourcequota/apis/resourcequota/install:go_default_library", | |||
"//plugin/pkg/admission/resourcequota/apis/resourcequota/v1alpha1:go_default_library", |
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.
Shouldn't we have this line?
@@ -22,12 +22,12 @@ import ( | |||
"k8s.io/apimachinery/pkg/runtime" | |||
utilruntime "k8s.io/apimachinery/pkg/util/runtime" | |||
resourcequotaapi "k8s.io/kubernetes/plugin/pkg/admission/resourcequota/apis/resourcequota" | |||
resourcequotav1alpha1 "k8s.io/kubernetes/plugin/pkg/admission/resourcequota/apis/resourcequota/v1alpha1" |
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.
I think, we should have this line as well.
) | ||
|
||
// Install registers the API group and adds types to a scheme | ||
func Install(scheme *runtime.Scheme) { | ||
utilruntime.Must(resourcequotaapi.AddToScheme(scheme)) | ||
utilruntime.Must(resourcequotav1alpha1.AddToScheme(scheme)) |
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.
I think you should have this version but setting the default version to beta in subsequent line which you are doing should be good enough.
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.
actually both should be added in subsequent line in priority order. will update, Thanks!
afd71f6
to
5e384d4
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.
/lgtm
Thanks, @vikaschoudhary16!
/lgtm , thanks :) |
/assign @deads2k for approval. |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bsalamat, deads2k, k82cn, vikaschoudhary16 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 |
Automatic merge from submit-queue (batch tested with PRs 66351, 66883, 66156). If you want to cherry-pick this change to another branch, please follow the instructions here. |
ref: kubernetes/enhancements#587
Release note:
/sig node
/sig scheduling
/cc @derekwaynecarr @deads2k @liggitt @sjenning @aveshagarwal @ravisantoshgudimetla @smarterclayton