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

backporting: Suggest only one related commit for a backport #16907

Merged
merged 1 commit into from Jul 28, 2021

Conversation

joestringer
Copy link
Member

@joestringer joestringer commented Jul 15, 2021

Recently, the check-stable script has suggested every single possible
match for commits where the name does not uniquely identify the commit.
This can be a bit confusing to backporters since it looks like there are
many commits to backport as part of this PR, but the second and later
ones are not necessary to backport.

 * PR: 16589 -- vagrant: Bump all Vagrant box versions (@pchaigno) -- https://github.com/cilium/cilium/pull/16589
   Merge with 1 commit(s) merged at: Tue, 22 Jun 2021 12:36:17 -0700!       
     Branch:     master (!)                          refs/pull/16589/head   
                 ----------                          -------------------    
     v (start)                                                              
     |  edf76fb1ef6b58d5ef90b439d54134f314ed086e                            
5bef5d77137a9ecc5d3f2b72149307ffdd52cd42                                    
4dc60e6faf654d7424ee959867a774205b3fed13                                    
816b3231cdbc39f4bcdd3e6f5b40a056459a478c                                    
51826b31087496d108044f3bffbf304580fffb4a                                    
df8238d451d755d5be75e202be89b4f88067c77b                                    
a4e7bc6c1f0e96078793458b6719b9a3999b89db via fb723f8133c40faa068a5a401f594622668b2753 ("vagrant: Bump all Vagrant box versions")
     v (end)                                                                

Probably within the last year of commits, we should be able to correlate
the exact commit that needs backporting, so iterate through those to
find the exact commit. If none of those are the correct commit, fail out
and push back to the backporter to figure out.

This allows us to now accurately pick the correct commit in most cases:

 * PR: 16589 -- vagrant: Bump all Vagrant box versions (@pchaigno) -- https://github.com/cilium/cilium/pull/16589
   Merge with 1 commit(s) merged at: Tue, 22 Jun 2021 12:36:17 -0700!       
     Branch:     master (!)                          refs/pull/16589/head   
                 ----------                          -------------------    
     v (start)                                                              
     |  edf76fb1ef6b58d5ef90b439d54134f314ed086e via fb723f8133c40faa068a5a401f594622668b2753 ("vagrant: Bump all Vagrant box versions")
     v (end)                                                                

Manually tested by substituting a known commit into 'related_commits',
and by checking the current v1.8 backports which includes an ambiguous
commit due to a revert+reapply in the master branch.

Fixes: #16572

@joestringer joestringer requested a review from a team as a code owner July 15, 2021 23:09
@joestringer joestringer added needs-backport/1.10 release-note/misc This PR makes changes that have no direct user impact. labels Jul 15, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jul 15, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.10.4 Jul 15, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.8.11 Jul 15, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.9.9 Jul 15, 2021
@joestringer joestringer requested review from pchaigno and removed request for aditighag July 15, 2021 23:09
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

🚀

Copy link
Member

@pchaigno pchaigno 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 this is creating ambiguity where there isn't:

Found the following ambiguous commits:
edf76fb paul@cilium.io 2021-06-18 vagrant: Bump all Vagrant box versions
5bef5d7 paul@cilium.io 2021-04-21 vagrant: Bump all Vagrant box versions
4dc60e6 paul@cilium.io 2021-04-19 vagrant: Bump all Vagrant box versions
816b323 paul@cilium.io 2020-11-25 vagrant: Bump all Vagrant box versions
51826b3 paul@cilium.io 2020-11-13 vagrant: Bump all Vagrant box versions
df8238d paul@cilium.io 2020-09-30 vagrant: Bump all Vagrant box versions
a4e7bc6 paul@cilium.io 2020-07-23 vagrant: Bump all Vagrant box versions

How are those commits ambiguous? They don't have the same author date at all. If they don't have the same author dates, they are not the same, regardless of commit titles.

Printing them all is only likely to confuse backporters IMO. What are they suppose to do with that information? Check every commit? If they should just check the dates match, why not automate that part?

@joestringer joestringer added this to Needs backport from master in 1.9.10 Jul 19, 2021
@joestringer joestringer removed this from Needs backport from master in 1.9.9 Jul 19, 2021
@joestringer joestringer marked this pull request as draft July 20, 2021 00:58
@joestringer
Copy link
Member Author

👍 good feedback @pchaigno , and thanks for being patient with me in Slack 😅

I plan to respin this some time this week.

Recently, the check-stable script has suggested every single possible
match for commits where the name does not uniquely identify the commit.
This can be a bit confusing to backporters since it looks like there are
many commits to backport as part of this PR, but the second and later
ones are not necessary to backport.

 * PR: 16589 -- vagrant: Bump all Vagrant box versions (@pchaigno) -- cilium#16589
   Merge with 1 commit(s) merged at: Tue, 22 Jun 2021 12:36:17 -0700!
     Branch:     master (!)                          refs/pull/16589/head
                 ----------                          -------------------
     v (start)
     |  edf76fb
5bef5d7
4dc60e6
816b323
51826b3
df8238d
a4e7bc6 via fb723f8 ("vagrant: Bump all Vagrant box versions")
     v (end)

Probably within the last year of commits, we should be able to correlate
the exact commit that needs backporting, so iterate through those to
find the exact commit. If none of those are the correct commit, fail out
and push back to the backporter to figure out.

This allows us to now accurately pick the correct commit in most cases:

 * PR: 16589 -- vagrant: Bump all Vagrant box versions (@pchaigno) -- cilium#16589
   Merge with 1 commit(s) merged at: Tue, 22 Jun 2021 12:36:17 -0700!
     Branch:     master (!)                          refs/pull/16589/head
                 ----------                          -------------------
     v (start)
     |  edf76fb via fb723f8 ("vagrant: Bump all Vagrant box versions")
     v (end)

Manually tested by substituting a known commit into 'related_commits',
and by checking the current v1.8 backports which includes an ambiguous
commit due to a revert+reapply in the master branch.

Signed-off-by: Joe Stringer <joe@cilium.io>
@joestringer joestringer force-pushed the submit/improve-backport-script branch from 2fe42ea to 4fe4e41 Compare July 21, 2021 22:32
@joestringer joestringer changed the title backporting: Highlight ambiguous backport suggestions backporting: Suggest only one related commit for a backport Jul 21, 2021
@joestringer joestringer marked this pull request as ready for review July 21, 2021 22:34
@joestringer joestringer added this to Needs backport from master in 1.8.12 Jul 22, 2021
@joestringer joestringer removed this from Needs backport from master in 1.8.11 Jul 22, 2021
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

LGTM, with one suggestion below.

contrib/backporting/check-stable Show resolved Hide resolved
@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 27, 2021
@brb brb merged commit 9abbbbf into cilium:master Jul 28, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.10.4 Jul 28, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.9 in 1.9.10 Jul 28, 2021
@joestringer joestringer deleted the submit/improve-backport-script branch August 9, 2021 20:55
@joestringer joestringer moved this from Needs backport from master to Backport done to v1.8 in 1.8.12 Sep 1, 2021
@joestringer joestringer added this to Backport pending to v1.9 in 1.9.11 Sep 1, 2021
@joestringer joestringer removed this from Backport pending to v1.9 in 1.9.10 Sep 1, 2021
@joestringer joestringer removed this from Backport pending to v1.9 in 1.9.11 Sep 1, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.10 in 1.10.4 Sep 1, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.10 in 1.10.4 Sep 1, 2021
@joestringer joestringer added this to Backport done to v1.9 in 1.9.10 Sep 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.10.4
Backport done to v1.10
1.8.12
Backport done to v1.8
1.9.10
Backport done to v1.9
Development

Successfully merging this pull request may close these issues.

None yet

6 participants