GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Validate a branch that we parse when running cherry_picker --continue by abadger · Pull Request #266 · python/core-workflow

Conversation

abadger commented Jun 16, 2018

Copy link

Copy Markdown

Contributor

When running cherry_picker --continue we count on being able to get
certain information from the branch's name which cherry_picker
constructed earlier. Verify as best we can that the branch name is one
which cherry_picker could have constructed.

Relies on the changes here: #265
(which does the work of one of the validations)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

abadger requested a review from Mariatta as a code owner

June 16, 2018 23:07

the-knights-who-say-ni added the CLA signed label

Jun 16, 2018

webknjaz reviewed

Jun 18, 2018

View reviewed changes

Comment thread

cherry_picker/cherry_picker/cherry_picker.py


		if prefix != 'backport':
		raise ValueError('branch name is not prefixed with "backport-". Is this a cherry_picker branch?')
		if not re.match('[0-9a-f]{7,40}', sha):

webknjaz Jun 18, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Could also validate the sha against git (like it's done with a commit in config)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

abadger Jun 21, 2018

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Done.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

abadger force-pushed the validate-branch-names branch from 5ffb9e7 to b88b167 Compare

June 22, 2018 04:16

abadger closed this

Jun 22, 2018

abadger reopened this

Jun 22, 2018

abadger commented Jun 22, 2018

Copy link

Copy Markdown

Contributor Author

Ready for review

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

webknjaz reviewed

Jun 22, 2018

View reviewed changes

Comment thread

cherry_picker/cherry_picker/cherry_picker.py Outdated

		except AttributeError as attr_err:
		raise ValueError(f'Branch {branch} seems to not have a version in its name.') from attr_err

		"""Return the branches to cherry-pick to, sorted by version"""

webknjaz Jun 22, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Nitpick: I prefer PEP 257 compatible docstrings. Please add a period at the end of the sentence.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

webknjaz reviewed

Jun 22, 2018

View reviewed changes

Comment thread

cherry_picker/cherry_picker/cherry_picker.py Outdated

		if not re.match('[0-9a-f]{7,40}', sha):
		raise ValueError(f'branch name has an invalid sha: {sha}')

		cmd = ['git', 'log', '-r', sha]

webknjaz Jun 22, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Nitpick: CherryPicker.check_repo() has 99% same code. I'd reuse that.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

abadger Jun 22, 2018

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I thought about this but something struck me as off about doing that and I couldn't quite put my finger on it yesterday. This morning I realized it's because check_repo() is not well encapsulated. The sha is contained as an attribute on the CherryPicker object but the repository is coming from the current working directory. This wasn't immediately obvious because cherry_picker doesn't give you the option to work on a repository that's not the current working directory but when I thought about making the code generic to both places it triggered my sense that something was wrong.

Now that I know that's what struck me as wrong, I'd like to go ahead and move this to its own function in this PR and submit a second PR that allows setting the working directory from which all the git commands are run. When I do that, though, functions like get_base_branch() will either need to become methods of CherryPicker or I'll have to pass the path to the repository all the way through the stack. Does that future plan sound okay?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

webknjaz reacted with thumbs up emoji

All reactions

👍 1 reaction

webknjaz Jun 22, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Sounds good

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

webknjaz approved these changes

Jun 22, 2018

View reviewed changes

webknjaz left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Looks perfect! (besides those nitpicks, it's up to you whether to fix them)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

webknjaz approved these changes

Jun 22, 2018

View reviewed changes

webknjaz commented Jul 2, 2018

Copy link

Copy Markdown

Member

Hi @Mariatta, could you please review this? It seems complete to me.

Mariatta reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Mariatta commented Jul 9, 2018

Copy link

Copy Markdown

Member

There is merge conflict now.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

abadger added 2 commits

July 8, 2018 21:38


          Validate a branch that we parse when running cherry_picker --continue

113eb21

When running cherry_picker --continue we count on being able to get
certain information from the branch's name which cherry_picker
constructed earlier.  Verify as best we can that the branch name is one
which cherry_picker could have constructed.

Relies on the changes here: python#265
(which does the work of one of the validations)


          Extract the validation of a sha within the repo to a common function

4eba21f

abadger force-pushed the validate-branch-names branch from 1b467bc to 4eba21f Compare

July 9, 2018 04:38

abadger commented Jul 9, 2018

Copy link

Copy Markdown

Contributor Author

Updated.

webknjaz reacted with hooray emoji

All reactions

🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

webknjaz approved these changes

Jul 9, 2018

View reviewed changes

Mariatta approved these changes

Jul 13, 2018

View reviewed changes

Mariatta merged commit c341234 into python:master

Jul 13, 2018

Mariatta commented Jul 13, 2018

Copy link

Copy Markdown

Member

Thanks for the patience! Just finally have a little extra time to review this :)

webknjaz reacted with hooray emoji

All reactions

🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

abadger deleted the validate-branch-names branch

July 18, 2018 19:33

abadger commented Jul 18, 2018

Copy link

Copy Markdown

Contributor Author

Thanks Mariatta!

webknjaz reacted with hooray emoji

All reactions

🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Mariatta mentioned this pull request

Aug 3, 2018

Please update the changelog for cherry_picker #279

Closed

Mariatta added the cherry_picker.py label

Aug 3, 2018

Reviewers

webknjaz

webknjaz approved these changes

Mariatta

Mariatta approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

cherry_picker.py CLA signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.