Skip to content

Conversation

@liggitt
Copy link
Member

@liggitt liggitt commented Feb 15, 2019

The currently described process was formed prior to keps/tracking issues being required for all kubernetes enhancements.

It also relied on manually created issues replicating info already present in other places.

Over the past several months, the manual steps required to keep a secondary issue list current proved problematic.

This PR makes the following changes:

  • adds whitespace and reorders some sections (separate commit, no content changes)
  • simplifies the overview/goals section
  • collapses the prereqs/submitters/reviewers/results sections into a single mechanics section with a clearer workflow:
    • proposes using a project board in the kubernetes org to improve visibility and issue triage query capability
    • proposes using a dedicated api-review label to indicate an API review is needed on any issue or PR in the kubernetes org
    • describes a review workflow that allows using existing github issues/PRs, and existing github and PR dashboard queries to get visibility to assigned issues, review queue, and review status

Two well-understood bot changes are proposed to make this workflow possible and easier to engage with:

  • /api-review and /api-review cancel commands to let org members request or cancel API review requests
  • an auto-commenter that comments on PRs/issues labeled with kind/api-change referencing this document and noting the steps to request a review

follow-ups:

  • revise the moderator section, since it hasn't been staffed yet (possibly coordinate with sig-pm to help there)
  • update the pool expansion section to route requests through k/org instead of the mailing lists (and update templates for approver/reviewer requests/requirements there)

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. labels Feb 15, 2019
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 15, 2019
@liggitt
Copy link
Member Author

liggitt commented Feb 15, 2019

cc @justaugustus @jdumars

still WIP, but this is a stab at simplifying the described workflow to minimize duplicate issues that have to be created, and get the board tracking the things needing reviewing more directly.

@liggitt liggitt force-pushed the api-review branch 2 times, most recently from dfa9ffc to e7840d3 Compare February 15, 2019 20:59
@liggitt liggitt changed the title WIP - Update api review process mechanics Update api review process mechanics Feb 15, 2019
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 15, 2019
@liggitt
Copy link
Member Author

liggitt commented Feb 15, 2019

cc @kubernetes/sig-contributor-experience-feature-requests for /api-review and /api-review cancel proposal

@k8s-ci-robot k8s-ci-robot added sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. kind/feature Categorizes issue or PR as related to a new feature. labels Feb 15, 2019
@liggitt
Copy link
Member Author

liggitt commented Feb 15, 2019

cc @kubernetes/api-reviewers @kubernetes/api-approvers

@liggitt liggitt force-pushed the api-review branch 3 times, most recently from 7a6f21d to 1a1756c Compare February 15, 2019 21:30
@justaugustus
Copy link
Member

/cc

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.

@liggitt -- thanks so much for getting updated documentation out so quickly! :)

Some thoughts...

  • As the /api-review command doesn't yet exist, can we explicitly say that it's coming soon? I know it's mentioned later in the doc as a potential process improvement, but right now this reads like it exists today, which would be confusing to people seeking reviews.

    (In favor of adding that command, btw. Should be simple enough with what's already in test-infra.)

  • Should we automatically label changes that touch APIs as kind/api-change?

  • API approvers request process could/should flow through k/org. It's the canonical source for org changes and this process should reflect that too. Maybe a new issue template specifically for these requests that outlines what you've stated here?

  • It's great that we're removing the diagram here as it's out of date, but we're orphaning it in the repo. Can we either outright delete the file or file an issue to update the diagram?

  • Would it be valuable to have a separate issue template in k/enhancements, specifically for API changes?

  • Moderators falls close in line with what we (SIG PM) had in mind as a goal in our forthcoming charter (end of February) i.e., grooming of cross-project backlogs (Steering, horizontal SIGs). Does it make sense for us to assist here?

@liggitt
Copy link
Member Author

liggitt commented Feb 16, 2019

  • As the /api-review command doesn't yet exist, can we explicitly say that it's coming soon? I know it's mentioned later in the doc as a potential process improvement, but right now this reads like it exists today, which would be confusing to people seeking reviews.

sure

  • Should we automatically label changes that touch APIs as kind/api-change?

we already do

  • API approvers request process could/should flow through k/org. It's the canonical source for org changes and this process should reflect that too. Maybe a new issue template specifically for these requests that outlines what you've stated here?

Let's update the pool expansion section in a follow-up. I agree that's where it should point, but that's separate from the review process mechanics I wanted to update with this PR.

  • It's great that we're removing the diagram here as it's out of date, but we're orphaning it in the repo. Can we either outright delete the file or file an issue to update the diagram?

Done

  • Would it be valuable to have a separate issue template in k/enhancements, specifically for API changes?

I think a mention of the API conventions in the standard KEP template is probably best.

  • Moderators falls close in line with what we (SIG PM) had in mind as a goal in our forthcoming charter (end of February) i.e., grooming of cross-project backlogs (Steering, horizontal SIGs). Does it make sense for us to assist here?

Quite possibly.

@justaugustus
Copy link
Member

SGTM. Thanks again, @liggitt!
/sig pm
/lgtm
/approve

For SIG Arch approval:
/assign @jdumars @bgrant0607 @mattfarina

@k8s-ci-robot k8s-ci-robot added sig/pm lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 16, 2019
@jdumars
Copy link
Contributor

jdumars commented Feb 18, 2019

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jdumars, justaugustus, liggitt

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 18, 2019
@k8s-ci-robot k8s-ci-robot merged commit bdf09f9 into kubernetes:master Feb 18, 2019
@liggitt liggitt deleted the api-review branch February 18, 2019 14:24
* A [KEP](https://github.com/kubernetes/enhancements/blob/master/keps/0001-kubernetes-enhancement-proposal-process.md) and tracking issue in [kubernetes/enhancements](https://github.com/kubernetes/enhancements/) has been created for changes within the kubernetes-* org introducing:
* Any new resource type
* Any new version of a stable API
* Any new functionality added to a stable API as defined by SIG Architecture and the API Reviewers
Copy link
Contributor

Choose a reason for hiding this comment

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

Should also include Any change to the meaning, validation, or behavior of a field

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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. 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. sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. 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.

8 participants