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.

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

Revise KEP template #703

Merged
merged 3 commits into from
Feb 8, 2019
Merged

Conversation

justaugustus
Copy link
Member

@justaugustus justaugustus commented Jan 19, 2019

Brings @pwittrock's updates in #690 over...

Update KEP template

  • Add release checklist
  • Expand upon graduation criteria
  • Add test plan section
  • Add upgrade / downgrade section
  • Add version skew section

Additionally:

  • Clarify some requirements in Release Team checklist
  • Do away with KEP number in template

Phil and I discussed earlier in the week that I'd pick this up, so he doesn't have to be responsible for iterating on any changes suggested.

/assign @calebamiles @spiffxp @pwittrock @bgrant0607
/sig pm architecture
/hold

Fixes: kubernetes/community#2245

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 19, 2019
@k8s-ci-robot k8s-ci-robot added sig/pm sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 19, 2019
@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 19, 2019
@justaugustus
Copy link
Member Author

/milestone v1.14

@k8s-ci-robot k8s-ci-robot added this to the v1.14 milestone Jan 20, 2019
@justaugustus
Copy link
Member Author

/remove-milestone v1.14
/milestone keps-beta

@justaugustus
Copy link
Member Author

Blockade for NEXT_KEP_NUMBER added in kubernetes/test-infra#10856.

aojea pushed a commit to aojea/enhancements that referenced this pull request Jan 22, 2019
KEP numbers will be obsolete once kubernetes#703 merges.
aojea pushed a commit to aojea/enhancements that referenced this pull request Jan 22, 2019
KEP numbers will be obsolete once kubernetes#703 merges.
@ixdy
Copy link
Member

ixdy commented Jan 23, 2019

Lines 35-38 still reference "KEP number", as does the YAML template.

@spiffxp
Copy link
Member

spiffxp commented Jan 24, 2019

Aside from @ixdy 's nit this LGTM

keps/0000-kep-template.md Outdated Show resolved Hide resolved
keps/0000-kep-template.md Outdated Show resolved Hide resolved
keps/0000-kep-template.md Outdated Show resolved Hide resolved
@spiffxp
Copy link
Member

spiffxp commented Feb 7, 2019

/unassign @derekwaynecarr
/cc @derekwaynecarr
FYI @timothysc this is how we suggest requesting review, it shows up on https://gubernator.k8s.io/pr just as much as /assign does

@spiffxp
Copy link
Member

spiffxp commented Feb 7, 2019

/cc @lavalamp @michmike
per #703 (comment)

@spiffxp
Copy link
Member

spiffxp commented Feb 7, 2019

(also, meta point, this is way too many people to constructively drive forward in a small incremental step, this turned into a way way bigger PR than I had hoped we would start with)

Copy link
Contributor

@jbeda jbeda left a comment

Choose a reason for hiding this comment

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

Biggest comment is that this is no longer really a template but rather instructions. Perhaps break out into a new doc and/or update https://github.com/kubernetes/enhancements/blob/master/keps/0001-kubernetes-enhancement-proposal-process.md?

Or merge this into the first kep?

But don't synchronize on me though!

Check these off as they are completed for the Release Team to track. These checklist items _must_ be updated for the enhancement to be released.

- [ ] kubernetes/enhancements issue in release milestone, which links to KEP (this should be a link to the KEP location in kubernetes/enhancements, not the initial KEP PR)
- [ ] KEP approvers have set the KEP status to `implementable`
Copy link
Contributor

Choose a reason for hiding this comment

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

May not make sense to put that in this section fo the template/instructions but we can put it someplace.

Check these off as they are completed for the Release Team to track. These checklist items _must_ be updated for the enhancement to be released.

- [ ] kubernetes/enhancements issue in release milestone, which links to KEP (this should be a link to the KEP location in kubernetes/enhancements, not the initial KEP PR)
- [ ] KEP approvers have set the KEP status to `implementable`
Copy link
Member

Choose a reason for hiding this comment

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

I still don't really understand why the "approvers" and "reviewers" section of KEPs exist.

If it's about people who are supposed to approve/review the KEP itself, don't we have owners files for that? KEPs are in separate directories now.

If it's about people who are going to help approve and/or review the code changes (e.g., demonstrating that you've lined up sufficient bandwidth in the schedules of busy folks to actually get the changes made), it's probably named wrong.

Actually IMO it's totally ambiguous right now and that section of the KEP could either be renamed or have a comment to make it clear what it means.

Copy link
Member Author

@justaugustus justaugustus Feb 8, 2019

Choose a reason for hiding this comment

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

Part of the goal is disambiguate the KEP itself from the place it lives / things that operate over it.
If someone, for whatever crazy reason, printed out a KEP, would they, at a glance, be able to understand who authored, edited, reviewed, and approved it?

OWNERS leads me to looking up the OWNERS_ALIASES, neither or which are guaranteed to stay the same over time. If I needed to reach out to someone to discuss the KEP, I would need more context.

We can do better on making that more clear in the documentation. I've added an AI in #822.

Copy link
Member

Choose a reason for hiding this comment

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

My intent was that the leads of the area (e.g., SIG TLs, subproject owners) would assign specific reviewers and approvers for each KEP, so that there are specific people that can review/approve, shepherd, serve as points of contact, etc. Always assigning SIG TLs to these roles is unrealistic due to time constraints, and if it's just everyone in a SIG, then it's easy to fall through the cracks or to get conflicting guidance. Unclear OARP is one of our biggest problems.


Check these off as they are completed for the Release Team to track. These checklist items _must_ be updated for the enhancement to be released.

- [ ] kubernetes/enhancements issue in release milestone, which links to KEP (this should be a link to the KEP location in kubernetes/enhancements, not the initial KEP PR)
Copy link
Member

Choose a reason for hiding this comment

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

Honestly from the perspective of people wanting to file KEPs this seems like obscure busywork. Why don't we make a bot to file these issues?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that's something we can start to look at, once we've stabilized more of the process.
We'd need to clean the metadata for all KEPs and scrub the existing Enhancement Issues beforehand. I've added an AI in #822.


- [ ] kubernetes/enhancements issue in release milestone, which links to KEP (this should be a link to the KEP location in kubernetes/enhancements, not the initial KEP PR)
- [ ] KEP approvers have set the KEP status to `implementable`
- [ ] Design details are appropriately documented
Copy link
Member

Choose a reason for hiding this comment

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

Are KEPs design docs or requirements docs?

People have said "both" but I'm not convinced that's a good answer. There is no reason to suppose that the set of people who know what good goals are has a lot of overlap with the set of people who will be good at charting a path to achieving the goals.

I personally feel that it's reasonable to hold a vote on goals (i.e. requirements), as long as they're appropriately phrased (e.g., "is problem X an important problem for the project to solve" NOT "is changing the frobber API to interoperate with the thingy a good idea"). It is not a good idea to vote on solutions (designs), that should be handled by the right technical folks. (I think any formal specification of this distinction can be gamed, unfortunately.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have a great answer for this at the moment. I think that "both" is the right answer currently, simply because all SIGs have slightly different operating mechanisms, each KEP isn't strictly technical content (it may be process-related, like this one), etc.

Authors and editors have the obvious roles.
I see reviewers as those who can help tighten the scope of a KEP and better define if it's something that needs heavy technical insight. Depending on the scope, those reviewers could very well be technical reviewers.

Approvers then act as the rubber stamp to signal SIG acceptance.

I've added an AI in #822 to firm up docs on this.

Copy link
Member

Choose a reason for hiding this comment

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

Requirements vs design: They must cover both. Whether requirements should be agreed upon before discussing the design is more of a process question, but it also depends on the complexity of the change. Simple changes could address both simultaneously.


##### Beta -> GA Graduation

- N examples of real world usage
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to have comments in markdown? Perhaps leave these as suggestions inside a comment; outside of the comment I'd say something like "TBD after beta" with the hint that people will update the KEP once they are in beta and know what they'll do?

Copy link
Member Author

@justaugustus justaugustus Feb 8, 2019

Choose a reason for hiding this comment

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

This section is only meant as examples, so authors should feel free to remove.

Signed-off-by: Stephen Augustus <[email protected]>
@justaugustus
Copy link
Member Author

@jbeda -- agreed that this has more instructions that the initial intent. I've taken an AI to pull those out and move them more top-level after this merges.

--

Given the spirit of merge early, iterate often (as @spiffxp was alluding to) do we think this is in a decent place to merge and iterate?
It's significantly better than the previous one, thanks to all of the reviews here.

I've captured the review comments that need to addressed in #822, and I can start knocking them out once this lands.

WDYT?

@jbeda
Copy link
Contributor

jbeda commented Feb 8, 2019

@justaugustus SGTM! Ship it!

@justaugustus
Copy link
Member Author

Alrighty, releasing the hold.
/hold cancel

The next /lgtm will ship it!

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 8, 2019
Copy link
Member

@idvoretskyi idvoretskyi left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 8, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: idvoretskyi, justaugustus

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

@idvoretskyi
Copy link
Member

@justaugustus you got it!

@k8s-ci-robot k8s-ci-robot merged commit 9bd0211 into kubernetes:master Feb 8, 2019
@spiffxp
Copy link
Member

spiffxp commented Feb 9, 2019

thanks for moving forward on this


[umbrella issues]: https://github.com/kubernetes/kubernetes/issues/42752
Consider the following in developing a version skew strategy for this enhancement:
- Does this enhancement involve coordinating behavior in the control plane and in the kubelet? How does an n-2 kubelet without this feature available behave when this feature is used?
Copy link
Member

Choose a reason for hiding this comment

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

This applies to nodes generally, so kube-proxy and any other node agents as well as kubelet.

@justaugustus justaugustus added area/enhancements Issues or PRs related to the Enhancements subproject and removed sig/pm labels Apr 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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/architecture Categorizes an issue or PR as relevant to SIG Architecture. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

KEP numbering is broken