Validate a branch that we parse when running cherry_picker --continue #266
Uh oh!
There was an error while loading. Please reload this page.
Conversation
| ifprefix!='backport': | ||
| raiseValueError('branch name is not prefixed with "backport-". Is this a cherry_picker branch?') | ||
| ifnotre.match('[0-9a-f]{7,40}', sha): |
There was a problem hiding this comment.
Could also validate the sha against git (like it's done with a commit in config)
abadgerforce-pushed the validate-branch-names branch from 5ffb9e7 to b88b167Compareabadger commented Jun 22, 2018
Ready for review |
| exceptAttributeErrorasattr_err: | ||
| raiseValueError(f'Branch {branch} seems to not have a version in its name.') fromattr_err | ||
| """Return the branches to cherry-pick to, sorted by version""" |
There was a problem hiding this comment.
Nitpick: I prefer PEP 257 compatible docstrings. Please add a period at the end of the sentence.
| ifnotre.match('[0-9a-f]{7,40}', sha): | ||
| raiseValueError(f'branch name has an invalid sha: {sha}') | ||
| cmd= ['git', 'log', '-r', sha] |
There was a problem hiding this comment.
Nitpick: CherryPicker.check_repo() has 99% same code. I'd reuse that.
There was a problem hiding this comment.
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?
webknjaz left a comment
webknjaz left a comment There was a problem hiding this comment.
Looks perfect! (besides those nitpicks, it's up to you whether to fix them)
webknjaz commented Jul 2, 2018
Hi @Mariatta, could you please review this? It seems complete to me. |
Mariatta commented Jul 9, 2018
There is merge conflict now. |
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)
abadgerforce-pushed the validate-branch-names branch from 1b467bc to 4eba21fCompareabadger commented Jul 9, 2018
Updated. |
Mariatta commented Jul 13, 2018
Thanks for the patience! Just finally have a little extra time to review this :) |
abadger commented Jul 18, 2018
Thanks Mariatta! |
4 participants
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)