Add PRR questionnaire to KEP template, require for targeting

[johnbelamaric]

This PR adds the PRR questionnaire as a formal part of the KEP, and makes it required when targeting the enhancement at a release.

A follow up PR in k/community will remove the questionnaire from there and point folks here.

/sig architecture
/sig release
/assign @wojtek-t @deads2k @justaugustus

[johnbelamaric]

/assign @jeremyrickard @mrbobbytables

[wojtek-t]

One thing that is not clear for me is who is allowed to approve it. Do we fully delegate to KEP approvers?

[johnbelamaric]

good point. I don't think so. we can use an OWNERs file with a regexp.

Process-wise, sig-scalability probably needs to approve the scalability section. In the PRR KEP we discuss a team similar to API review team. Perhaps we need to implement that before merging this.

[wojtek-t]

Given you move it here, we probably should remove it from the community repo, to avoid maintaining the same thing in two different places.

[johnbelamaric]

Yes, that's the plan. Different repo though so it will be a separate PR.

[justaugustus]

/hold for my review

@johnbelamaric -- On an initial glance, I'm wondering if there's a good reason for the PRR to not just be a section in the README.

Apologies if I'm lacking context from another conversation.

[johnbelamaric]

@justaugustus that was the initial thought, but I split it out for two reasons: 1) it is long; 2) it should have a separate set of approvers. But that latter process is ill defined at the moment.

[justaugustus]

@johnbelamaric --

1. it is long

I think this is fine, especially if it means that there will be more eyes on the PRR section.

2. it should have a separate set of approvers. But that latter process is ill defined at the moment.

Having a separate approver set is impossible today with the way OWNERS work.
We could move PRRs to their own directory, but I don't necessarily like the idea of having them too far from the KEP.

We could have the PRR approver added to approvers set instead and have that be a requirement for status change?

approvers:
  - "@johnbelamaric" # PRR Approver

[johnbelamaric]

Ok, for some reason I was thinking we could do regexp for file names in OWNERS files. Not sure where I got that idea...

I don't want to move it to its own directory either. I think your suggestion is good, I'll update the PR.

[johnbelamaric]

Ok, I added it directly, and also mentioned that it needs a PRR approver. But we should hold this until all that is set up.

[wojtek-t]

Regarding the tooling, I have no idea how the existing tooling works, so I may be completely wrong, but I think it hopefully should be a small thing to do. How I envision this to work is:

1. add another PRR_OWNERS file (or extend the existing one - thanks @johnbelamaric for this thought) [we can probably start with just top-level one
2. introduce prr-approved label (naming to be done) based on /prr-approve comment from PRR_OWNERS (hopefully that's only a matter of configuring existing tooling?)
3. change tide for this repo to require both approve and prr-approve labels?

As @johnbelamaric pointed out we will probably need to also auto-assign prr-approvers. Though I think that as a first step we can probably ask sig-leads (who are the only approvers for now) to add someone manually.
The remaining thing is that not every PR should require PRR-review. But OTOH, those trivial ones will probably be easy to review and will not add significant overhead.

@spiffxp @cblecker - does that above makes any sense from the tooling perspective? How much work would it be to make that happen? [If you're not the right people to answer, can you route it to the right ones?]

[spiffxp]

I'm not sure I'm a fan of a coding something specifically into prow for this purpose. I agree we're seeing cases for more flexible review/approval workflows than what we support right now, but it's unclear what the right path forward is, and nobody has improving this as a priority. Need some time to think about this. @cjwagner @fejta do you have any ideas?

A mechanism you could try to use today is to force KEPs going to release to use a directory based approach (I'm not clear if that's the standard now or if we're still putting everything in one file), put the PRR in subdir of the KEP, and use a no_parent_owners directive + approves to restrict approval of the PRR to your team/alias.

[wojtek-t]

That OWNERS file will be created by the KEP author - can we somehow enforce that it will be added and will have the no-parent stuff?

[justaugustus]

We can definitely add something as a presubmit to enforce this!

[justaugustus]

I'd agree with @spiffxp that no_parent_owners is favorable over creating a new workflow here.

[johnbelamaric]

I updated this based on @wojtek-t 's changes to the PRR questionnaire. I think we can merge this as-is, with these follow ups needed:

1. Remove the production-readiness.md from the community repo.
2. Add a prr-review team (in OWNERS_ALIASES)
3. Add KEP tooling to verify a PRR reviewer is present on a KEP

[neolit123]

missing noun?

[neolit123]

this seems like an overall improvement for gating features with respect to quality control, so thank you @johnbelamaric and the PRR project for adding this to the template. i did early reviews of this work but lost the trail later.

one minor complain that i have, after trying to fill the answers in a WIP KEP is that the majority of the questions are not really applicable to e.g. a new {kubectl|kubeadm} ... command that performs a one time operation instead of persisting a feature that affects cluster behavior.

i guess in such cases there must be some sort a default answer, so i just went with "Not applicable" in a few places, but still trying to clarify "why".

something else is that possibly some of the details next to questions could have been commented out <!-- ... --> so that the questionnaire focuses on the questions and available options. once we start publishing KEPs to a website some of those details will be hidden, similarly to the rest of the template.

thanks.

[neolit123]

typo in loogging

[neolit123]

there is no such field in the Proposals structure, causing kepval / make verify to fail:
https://github.com/kubernetes/enhancements/blob/master/pkg/kepval/keps/proposals.go#L49-L76

looks like the template itself is not validated.

[johnbelamaric]

this has since been fixed, thanks