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

Improve krel release-notes local k/sig-release repository handling #1126

Merged
merged 1 commit into from
May 7, 2020
Merged

Improve krel release-notes local k/sig-release repository handling #1126

merged 1 commit into from
May 7, 2020

Conversation

JamesLaverack
Copy link
Member

@JamesLaverack JamesLaverack commented Feb 22, 2020 •
edited
Loading

A number of changes to allow a user to run the krel release-notes multiple times. You'll still get an error if you already have a branch with the expected name on your fork, but it'll correctly manage the local checkout of k/sig-release now, including checking out the master branch to apply changes to, setting upstreams when pushing branches, and handling unsaved changes.

This also fixes a bug where the command wouldn't work at all as it attempted to use a git repository URL without SSH configuration.

Modify the krel release-notes subcommand to always create clean checkouts of k/sig-release and k-sigs/release-notes instead of re-using existing clones. Removed --kubernetes-sigs-fork-path and --sigrelease-fork-path flags.

@k8s-ci-robot k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/release-eng Issues or PRs related to the Release Engineering subproject sig/release Categorizes an issue or PR as relevant to SIG Release. labels Feb 22, 2020
@JamesLaverack
Copy link
Member Author

/assign @saschagrunert
/sig release

saschagrunert
saschagrunert reviewed Feb 22, 2020
View reviewed changes
@justaugustus
Copy link
Member

@JamesLaverack -- Have you had a chance to take a took at Sascha's comments?

@puerco puerco mentioned this pull request Mar 17, 2020
Merged
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 23, 2020
@JamesLaverack JamesLaverack force-pushed the krel-release-notes-changes branch from b600b76 to 92b7709 Compare March 23, 2020 17:17
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 23, 2020
@JamesLaverack JamesLaverack force-pushed the krel-release-notes-changes branch from 92b7709 to 9b55390 Compare March 23, 2020 17:28
saschagrunert
saschagrunert reviewed Mar 23, 2020
View reviewed changes
Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

Alright, can you please squash your commits into one?

@JamesLaverack
Copy link
Member Author

Alright, can you please squash your commits into one?

I can do. What do you want me to do with the commit messages? I've tried to keep each individual commit logically distinct and correctly commented. Or do you mean to just do a normal squash and keep all the messages in a single commit?

@saschagrunert
Copy link
Member

Alright, can you please squash your commits into one?

I can do. What do you want me to do with the commit messages? I've tried to keep each individual commit logically distinct and correctly commented. Or do you mean to just do a normal squash and keep all the messages in a single commit?

Yeah I meant one single commit which documents all changes. It’s not that much code so I see no reason to split it up any further. :)

@JamesLaverack JamesLaverack force-pushed the krel-release-notes-changes branch 4 times, most recently from a91bfc0 to 84c716a Compare March 23, 2020 18:24
saschagrunert
saschagrunert approved these changes Mar 23, 2020
View reviewed changes
Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

/lgtm

Thank you again!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 23, 2020
@saschagrunert
Copy link
Member

@JamesLaverack can you please add a release notes block for your changes? :)

/kind cleanup

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Mar 23, 2020
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 23, 2020
@JamesLaverack
Copy link
Member Author

/retest

saschagrunert
saschagrunert approved these changes Mar 24, 2020
View reviewed changes
Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@JamesLaverack
Copy link
Member Author

/assign @dougm

@puerco
Copy link
Member

puerco commented Mar 25, 2020

/lgtm
Thanks @JamesLaverack :)

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 26, 2020
@JamesLaverack JamesLaverack force-pushed the krel-release-notes-changes branch from 84c716a to b43ac79 Compare April 2, 2020 01:35
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 2, 2020
@JamesLaverack
Copy link
Member Author

/retest

@JamesLaverack
Always use a clean checkout for clones during release-notes
0601453 
This avoids a large category of failure modes where an existing on-disk
repository is in an unexpected state. We also delete the repo afterwards
to avoid multiple runs filling up the disk.

This change therefore removes configuration flags for the fork
locations.

Also included:

* Set the upstream when pushing a new branch
* Use HTTP transport for git instead of SSH

  As we are now using the GitHub token for authentication, we need the
  HTTP URL and not the SSH one.

* Remove check for branch existing on remote

  The existing code does not work for a number of reasons:

  - It only checks the default remote, not the user's fork.
  - Any error returned would be treated as a success, not just a "not
    found".

  Even without this check there is no risk, as GitHub will simply reject
  the pushing of a new branch without `--force`, which we do not use.

* Use existing branch if it already exists

  The `-B` flag to `git checkout` will create the branch if it does not
  exist (existing beaviour) but will also use it if it does exist.
@JamesLaverack JamesLaverack force-pushed the krel-release-notes-changes branch from b43ac79 to 0601453 Compare April 2, 2020 01:50
@puerco
Copy link
Member

puerco commented Apr 2, 2020

Tests are green, nice :)

saschagrunert
saschagrunert approved these changes Apr 2, 2020
View reviewed changes
Copy link
Member

@saschagrunert saschagrunert 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 Apr 2, 2020
@puerco
Copy link
Member

puerco commented Apr 28, 2020

@justaugustus Could you give this PR your final approval?

@justaugustus
Copy link
Member

Thanks @JamesLaverack and sorry this one slipped!
/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JamesLaverack, justaugustus, puerco, saschagrunert

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 May 7, 2020
@k8s-ci-robot k8s-ci-robot merged commit a8fd0e2 into kubernetes:master May 7, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone May 7, 2020
@puerco puerco mentioned this pull request May 8, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saschagrunert saschagrunert saschagrunert approved these changes

@puerco puerco puerco approved these changes

@hasheddan hasheddan Awaiting requested review from hasheddan

@hoegaarden hoegaarden Awaiting requested review from hoegaarden

Assignees

@dougm dougm

@justaugustus justaugustus

@saschagrunert saschagrunert

@puerco puerco

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/release-eng Issues or PRs related to the Release Engineering subproject cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/release Categorizes an issue or PR as relevant to SIG Release. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.19
Development

Successfully merging this pull request may close these issues.
6 participants
@JamesLaverack @justaugustus @saschagrunert @puerco @k8s-ci-robot @dougm