-
Notifications
You must be signed in to change notification settings - Fork 1.6k
KEP-4762: Promote to beta stage #5636
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
KEP-4762: Promote to beta stage #5636
Conversation
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.
Multiple PRR beta questions are missing answers.
|
||
- Add a conformance test to `test/e2e` that verifies our implementation conforms to the expectation defined in the table within the #Design Details section. | ||
|
||
### Graduation Criteria |
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.
Above:
- can you explain why no integration tests are added?
- link to the e2e that you plan to promote to conformance, that's a beta requirement that this functionality have working e2e.
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.
Also in the graduation criteria, you've mentioned e2e being completed during alpha.
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.
Since this feature is not particularly complex, I think the e2e tests we've added are sufficient. Moreover, we already have some existing e2e cases related to hostname, and merging them is a natural progression.
I have already added e2e cases during the alpha phase, and I will include them in the KEP.
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.
@soltysh we have pretty good coverage between e2e and unit kubernetes/kubernetes#132558 , and since is mostly a node feature integrations will be mostly redundant with the unit for the api validation
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 trust both of you, but it's nice to have it written down in the document, why particular group of tests wasn't added. I see the link to e2e was added, so at least that. But it would be nice to put above explanation into the doc, this can be handled in followup.
e8c9f20
to
8ac5de9
Compare
/lgtm sig network (modulo the standing comments from PRR and the clarifications) |
8ac5de9
to
efe3d81
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
@HirazawaUi you need @soltysh PRR approval |
Ah, thank you for the reminder. I'm responsible for multiple KEPs in this cycle and got confused. I mistakenly thought this KEP had already received PRR approval. |
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.
/approve
the PRR section
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aojea, HirazawaUi, SergeyKanzhelev, soltysh 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 |
HostnameOverride
feature gate to beta stage