Skip to content
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.

Sign up for GitHub

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

Jump to bottom

pkg/kepval: enable PRR check for implemented KEPs #3007

Merged
merged 12 commits into from
Oct 20, 2021
Merged

pkg/kepval: enable PRR check for implemented KEPs #3007

merged 12 commits into from
Oct 20, 2021

Conversation

spiffxp
Copy link
Member

@spiffxp spiffxp commented Oct 8, 2021

Related:

This addresses the specific concern raised in
#2960 (comment),
namely that KEPs with status: implemented skip our automated PRR
check, along some KEPs to make it into the release without going through
production readiness review.

Enabling the check for implemented KEPs caused 115 KEPs to fail
validation, mostly because the require metadata that was added in #2672
was only added for provisional or implementable KEPs.

The majority of these were handled by making a similar set of
assumptions:

  • if it's status: implemented and missing latest-milestone, set to
    0.0, in order to exempt from the PRR check (for v1.21 and later)
  • if it's status: implemented and missing stage, use either:
    • whatever stage in milestone matches latest-milestone
    • OR (for the vast majority), assume stage: stable

I've included the script used to make these edits.

The remaining cases were handled manually commit-by-commit. There were
some legitimately missing PRR files, which I have tried to explain in
each commit, but I'll add review comments with links to the relevant
PR's that allowed these to happen.

I feel vaguely uncomfortable about assuming everything is just straight
to stage: stable, so please see if any of these seem off to you.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/enhancements Issues or PRs related to the Enhancements subproject area/provider/azure Issues or PRs related to azure provider kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/auth Categorizes an issue or PR as relevant to SIG Auth. labels Oct 8, 2021
@k8s-ci-robot k8s-ci-robot added sig/autoscaling Categorizes an issue or PR as relevant to SIG Autoscaling. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/release Categorizes an issue or PR as relevant to SIG Release. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. sig/windows Categorizes an issue or PR as relevant to SIG Windows. labels Oct 8, 2021
@spiffxp
Copy link
Member Author

spiffxp commented Oct 8, 2021

/assign @deads2k @ehashman @johnbelamaric @wojtek-t
PTAL

@spiffxp
Copy link
Member Author

spiffxp commented Oct 8, 2021

/hold
For comment

/uncc @alisondy @ahg-g
/cc @jeremyeder @justaugustus @kikisdeliveryservice @LappleApple @mrbobbytables

@k8s-ci-robot k8s-ci-robot removed the request for review from alisondy October 8, 2021 17:51
@spiffxp
Copy link
Member Author

spiffxp commented Oct 13, 2021

Now failing because #2939 moved a KEP to implemented without the corresponding PRR file

@jeremyrickard
Copy link
Contributor

Now failing because #2939 moved a KEP to implemented without the corresponding PRR file

IMO, we should merge your PR and then follow up on any items above in follow ups for individual KEPS so we can get this in to make sure there are not more instances of this.

@kikisdeliveryservice wdyt?

@spiffxp spiffxp mentioned this pull request Oct 13, 2021
Merged
@k8s-ci-robot k8s-ci-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Oct 13, 2021
@spiffxp
Copy link
Member Author

spiffxp commented Oct 13, 2021

I added a PRR file for #3007

We need a PRR approver to LGTM the PRR files as well

@spiffxp
Copy link
Member Author

spiffxp commented Oct 13, 2021

IMO, we should merge your PR and then follow up on any items above in follow ups for individual KEPS so we can get this in to make sure there are not more instances of this.

This blocks merges based on the new criteria, so this would require a manual merge, and all PR's would be blocked until the failures addressed. I'd rather try to fix things in this PR if possible. Apologies for the CI noise.

@jeremyrickard
Copy link
Contributor

@ehashman @johnbelamaric i

IMO, we should merge your PR and then follow up on any items above in follow ups for individual KEPS so we can get this in to make sure there are not more instances of this.

This blocks merges based on the new criteria, so this would require a manual merge, and all PR's would be blocked until the failures addressed. I'd rather try to fix things in this PR if possible. Apologies for the CI noise.

Yeah, totally. I was thinking fix thing so they pass for now, then revisit items that may need discussion. Thanks for working on this!

@spiffxp
Copy link
Member Author

spiffxp commented Oct 13, 2021

6f464ad adds an extra validation check for status:implemented == stage:stable, it turns out nothing else violates this so I'd rather keep it as a rule if you all think this is true (it was surprising to me to see this is how everyone interprets it, so I'd rather prevent myself from running into an invalid state space again)

spiffxp
spiffxp commented Oct 14, 2021
View reviewed changes
keps/prod-readiness/sig-auth/2907.yaml
Comment on lines +1 to +3
kep-number: 2907
stable:
approver: "@johnbelamaric"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#3007 moved this to implemented but missed PRR files

I think this is out of tree so maybe not explicitly covered by PRR

justaugustus
justaugustus approved these changes Oct 18, 2021
View reviewed changes
Copy link
Member

@justaugustus justaugustus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spiffxp -- Thanks for this!

Given the comments from other @kubernetes/enhancements-maintainers, this looks good to lift the hold.
/lgtm
/approve

@justaugustus
Copy link
Member

/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Oct 18, 2021
@justaugustus justaugustus added this to the keps-beta milestone Oct 18, 2021
@k8s-triage-robot
Copy link

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

johnbelamaric
johnbelamaric reviewed Oct 20, 2021
View reviewed changes
Copy link
Member

@johnbelamaric johnbelamaric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!! This will clean up a lot and prevent future misses.

/approve

keps/sig-cli/2206-openapi-features-in-kustomize/kep.yaml
status: implemented
status: implementable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The permabeta KEP is about served APIs - it implemented automatic disabling of apis that are beta for too many releases.

But the general principle applies beyond APIs - we really shouldn't be leaving stuff in a perma-beta state. If we mark this as implemented then it implies there is no more work to do, which isn't true. So I agree that this should be implementable.

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 20, 2021
@k8s-ci-robot k8s-ci-robot merged commit 9e5e92e into kubernetes:master Oct 20, 2021
@k8s-ci-robot k8s-ci-robot modified the milestones: keps-beta, v1.23 Oct 20, 2021
@spiffxp spiffxp deleted the prr-implemented branch November 1, 2021 19:52
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jeremyrickard, Jjajsja, johnbelamaric, justaugustus, spiffxp

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnbelamaric johnbelamaric johnbelamaric left review comments

@kikisdeliveryservice kikisdeliveryservice kikisdeliveryservice left review comments

@justaugustus justaugustus justaugustus approved these changes

@lasomethingsomething lasomethingsomething Awaiting requested review from lasomethingsomething

@mrbobbytables mrbobbytables Awaiting requested review from mrbobbytables

@jeremyrickard jeremyrickard Awaiting requested review from jeremyrickard

Assignees

@justaugustus justaugustus

@ehashman ehashman

@deads2k deads2k

@johnbelamaric johnbelamaric

@wojtek-t wojtek-t

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/enhancements Issues or PRs related to the Enhancements subproject area/provider/azure Issues or PRs related to azure provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/autoscaling Categorizes an issue or PR as relevant to SIG Autoscaling. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/release Categorizes an issue or PR as relevant to SIG Release. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.23
Development

Successfully merging this pull request may close these issues.
10 participants
@spiffxp @jeremyrickard @justaugustus @k8s-triage-robot @k8s-ci-robot @kikisdeliveryservice @johnbelamaric @ehashman @deads2k @wojtek-t