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

git-node: add --backport flag to land #383

Merged
merged 1 commit into from
Feb 14, 2020

Conversation

andrewhughes101
Copy link
Contributor

@andrewhughes101 andrewhughes101 commented Jan 23, 2020

This commit adds functionality to land that allows you to land backports
with the correct metadata

example: git node land --backport <PR number>

Co-authored-by: @AshCripps ashley.cripps@ibm.com

@andrewhughes101 andrewhughes101 force-pushed the backport-land branch 2 times, most recently from 3686797 to bc7c07d Compare January 27, 2020 10:10
@codecov
Copy link

codecov bot commented Jan 27, 2020

Codecov Report

Merging #383 into master will increase coverage by 0.28%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #383      +/-   ##
==========================================
+ Coverage   75.66%   75.95%   +0.28%     
==========================================
  Files          21       21              
  Lines        1422     1439      +17     
==========================================
+ Hits         1076     1093      +17     
  Misses        346      346
Impacted Files Coverage Δ
lib/links.js 89.85% <100%> (+1.33%) ⬆️
lib/metadata_gen.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ecf4b70...664345b. Read the comment docs.

@sam-github sam-github added the Work In Progress PR's that are in progress label Jan 27, 2020
@sam-github
Copy link
Contributor

@andrewhughes101 I labelled WIP because I think you intend to extend the tests now that the current ones are passing.

components/git/land.js Show resolved Hide resolved
test/fixtures/pr_with_backport.json Outdated Show resolved Hide resolved
components/git/land.js Outdated Show resolved Hide resolved
components/git/land.js Show resolved Hide resolved
lib/links.js Outdated Show resolved Hide resolved
lib/links.js Outdated Show resolved Hide resolved
lib/metadata_gen.js Outdated Show resolved Hide resolved
Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

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

LGTM, but I've never reviewed this code before, someone who actually has should at least have a look. Or... maybe unnecessary, since this is additive? As long as it works for @nodejs/lts, and @BethGriggs approves, perhaps that's enough?

@BethGriggs
Copy link
Member

Mostly LGTM. I've pulled down the code and tried pulling in a few backports and it works as expected 😃. The only one I had issues with was nodejs/node#31431 - it states that it cannot detect PR-URL (but the PR-URL is in each of the commits). Is the issue because the tool is looking for "PR-URL:" in the PR body rather than in the individual commit messages?

@AshCripps
Copy link
Member

Mostly LGTM. I've pulled down the code and tried pulling in a few backports and it works as expected 😃. The only one I had issues with was nodejs/node#31431 - it states that it cannot detect PR-URL (but the PR-URL is in each of the commits). Is the issue because the tool is looking for "PR-URL:" in the PR body rather than in the individual commit messages?

Yeah the tool itself doesn't have that functionality that we could see at least, it only goes through the data inside the PR message itself

@MylesBorins
Copy link
Member

Some of the commits in that PR had corrupt meta data... specifically they had PR-URL: #number-of-pr instead of the actual URL

@andrewhughes101 andrewhughes101 force-pushed the backport-land branch 3 times, most recently from 07df299 to 5320823 Compare February 12, 2020 16:50
@andrewhughes101
Copy link
Contributor Author

I've updated the PR to always place the Backport-PR-URL below the PR-URL in the metadata. I've also created some asciinema of this feature in action

Single commit: https://asciinema.org/a/300758
Multiple commits: https://asciinema.org/a/300808

lib/links.js Outdated Show resolved Hide resolved
lib/links.js Outdated Show resolved Hide resolved
This commit adds functionality to land that allows you to land backports
with the correct metadata

Co-authored-by: AshCripps <ashley.cripps@ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Work In Progress PR's that are in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants