-
Notifications
You must be signed in to change notification settings - Fork 450
Full tentative v1 API review #7282
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
e66cc1f to
27f1e93
Compare
27f1e93 to
6b1f938
Compare
|
/reopen |
|
@mimowo: Reopened this PR. 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-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mimowo 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 |
1 similar comment
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mimowo 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 |
|
/retest |
| @@ -0,0 +1,550 @@ | |||
| /* | |||
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.
config apis are not quite the same as the CRD API.
Is there a strong reason to graduate these in parallel?
I guess if we don't do it now we probably wouldn't get to it..
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 would propose graduating visibility separately as well.
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.
The purpose of the PR is to engage API reviewers early for preperation to V1, providing holistic overview of the API we have, which will all need to reach V1 eventually. Hopefully, we can address most of the feedback already in v1beta2.
As for the graduation itself, it is an open question. I know that technically we can graduate them separately, but I think it would be much easier for users to "be ready for graduation in 0.15", rather than to deal with the constant graduation over multiple releases.
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.
To make the purpose of the PR more clear I have moved the files under v1 already, and retitled the PR, and updated the "Special notes for your reviewer:" section.
|
@mimowo could you please fix verify test? |
59af778 to
28a0c3e
Compare
28a0c3e to
d1a4fc9
Compare
yes, the verify now passes with a bit of post processing to revert the auto-generated code. |
d1a4fc9 to
3f9f99a
Compare
|
rebased and dropped from the diff for now config API, and visibility API so that it is easier to focus on the main CRD changes (following the suggestion from wg-batch meeting) |
What type of PR is this?
/kind api-change
What this PR does / why we need it:
To allow reviewing the tentative full v1 API without exploding the diff by changes in the code.
Which issue(s) this PR fixes:
Related to #7113
Special notes for your reviewer:
The purpose of the PR is to allow review of the entire Kueue API for the API reviewers with the tentative goal of having good v1.
Our mid term goal is to have a good v1beta2 which will be as close to v1 as possible.
However, some of the proposals (more involving) might be deferred from v1beta2 for later before full v1.
This is how I prepared the PR:
pkg/controller/constants/constants.gointoapis/kueue/v1beta2/constants.goas they are de-facto part of the API by defining names of important labels and annotations.Does this PR introduce a user-facing change?