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

Add more milestone info to the release doc #4058

Merged
merged 3 commits into from
Dec 6, 2019

Conversation

jeefy
Copy link
Member

@jeefy jeefy commented Sep 5, 2019

First pass at addressing kubernetes/sig-release#320.

Feedback and nits appreciated. :)

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 5, 2019
@k8s-ci-robot k8s-ci-robot added area/developer-guide Issues or PRs related to the developer guide sig/release Categorizes an issue or PR as relevant to SIG Release. labels Sep 5, 2019
Copy link
Contributor

@geekygirldawn geekygirldawn left a comment

Choose a reason for hiding this comment

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

Pointing out a little typo.

contributors/devel/sig-release/release.md Outdated Show resolved Hide resolved
- kubernetes/release
- kubernetes/sig-release
- kubernetes/website

Copy link
Contributor

Choose a reason for hiding this comment

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

I am curious - do all patches get milestoned these days? I'd love some clarifying language here!

Copy link
Member

Choose a reason for hiding this comment

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

Once merged, a PR is milestoned to the milestone in which it merged.

Copy link
Member

Choose a reason for hiding this comment

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

Also cherry-pick patches must have the milestone associated with the branch to which they are targeted. I've proposed some additional detail on this front via #4081 .

Copy link
Member

@tpepper tpepper Nov 15, 2019

Choose a reason for hiding this comment

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

Given the automation on release branches, users never are interacting with the milestone on release branches. If you make a PR against release-1.16 branch, it is milestoned as 1.16. I think it clutters the documentation to have separate treatment then. If it's worded as:

**On PRs against the master branch**, milestones are auto-applied so
this is less necessary. 

**On PRS against anything not master** milestones are auto-applied so
this is less necessary.

Which then can be condensed to:

** On PRs**, milestones are auto-applied so
this is less necessary.

Which I don't think is the desire here.

I think what we actually have is:

  • At creation time, PRs against the master branch need humans to hint at which milestone they might want the PR to target.
  • Once merged, PRs against the master branch have milestones auto-applied so from that time onward
    human management of that PR's milestone is less necessary.
  • On PRs against anything not master branch, milestones are auto-applied when the PR is created so
    no human management of the milestone is less ever necessary.

@guineveresaenger
Copy link
Contributor

guineveresaenger commented Sep 11, 2019

this is good to have!
edit: dang I can't slash-approve in this folder heh

@cblecker
Copy link
Member

/assign @justaugustus

@tpepper
Copy link
Member

tpepper commented Sep 11, 2019

This looks good to me, but I'll hold off with the lgtm command to get additional review.

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.

So here's the tricky thing...
Now that we have the milestone_applier plugin, it changes things a little.

Namely, any PR that merges within a certain timeframe will automatically get milestoned according to the rules here:
https://github.com/kubernetes/test-infra/blob/1a3702547e56328af9e764a3ac1ff8b1aa98a532/prow/plugins.yaml#L251-L265

So it isn't a strict requirement to have a milestone on the master branch, unless you definitely want the Release Team to watch the PR. This is only true of PRs, not issues.

On the patch release branches, the milestone is absolutely a requirement.

Ideally, I'd love it if everything was milestoned, but I'm just calling out the fact that not everything needs to be with the introduction of that plugin.

Let's get some more discussion going here before merging.

@guineveresaenger
Copy link
Contributor

Let's reword, encompassing the following points (just trying to capture my best understanding of the situation)

  • on issues milestones must be applied correctly, via triage by the SIG, so that rel team can follow bugs and enhancements (any enhancement related issue needs a milestone)
  • on PRs against master milestones are auto-applied so this is less necessary
  • on PRS against anything not master the milestone requirement is absolute and iron-clad necessary

TL;DR: milestone early, milestone often, and everyone will be happier. 😛

@guineveresaenger
Copy link
Contributor

I wasn't going for using my somewhat casual language here 😅 but I think it's clear, and I like it.

/lgtm

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

@justaugustus or @tpepper for approval?

Copy link
Member

@tpepper tpepper left a comment

Choose a reason for hiding this comment

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

I think the edit confused things.

- kubernetes/release
- kubernetes/sig-release
- kubernetes/website

Copy link
Member

@tpepper tpepper Nov 15, 2019

Choose a reason for hiding this comment

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

Given the automation on release branches, users never are interacting with the milestone on release branches. If you make a PR against release-1.16 branch, it is milestoned as 1.16. I think it clutters the documentation to have separate treatment then. If it's worded as:

**On PRs against the master branch**, milestones are auto-applied so
this is less necessary. 

**On PRS against anything not master** milestones are auto-applied so
this is less necessary.

Which then can be condensed to:

** On PRs**, milestones are auto-applied so
this is less necessary.

Which I don't think is the desire here.

I think what we actually have is:

  • At creation time, PRs against the master branch need humans to hint at which milestone they might want the PR to target.
  • Once merged, PRs against the master branch have milestones auto-applied so from that time onward
    human management of that PR's milestone is less necessary.
  • On PRs against anything not master branch, milestones are auto-applied when the PR is created so
    no human management of the milestone is less ever necessary.

@guineveresaenger
Copy link
Contributor

@jeefy - PTAL?

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 6, 2019
@jeefy
Copy link
Member Author

jeefy commented Dec 6, 2019

@guineveresaenger Thanks for the ping. Updated w/ @tpepper prose. :)

@justaugustus
Copy link
Member

Thanks y'all!
/lgtm
/approve
/milestone v1.17

@k8s-ci-robot
Copy link
Contributor

@justaugustus: The provided milestone is not valid for this repository. Milestones in this repository: [August, Next]

Use /milestone clear to clear the milestone.

In response to this:

Thanks y'all!
/lgtm
/approve
/milestone v1.17

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jeefy, 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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 6, 2019
@k8s-ci-robot k8s-ci-robot merged commit 91187c1 into kubernetes:master Dec 6, 2019
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/developer-guide Issues or PRs related to the developer guide cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/release Categorizes an issue or PR as relevant to SIG Release. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants