Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

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

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

Merged
merged 2 commits into from
Jul 13, 2018
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
113eb21
Validate a branch that we parse when running cherry_picker --continue
abadger Jun 16, 2018
4eba21f
Extract the validation of a sha within the repo to a common function
abadger Jun 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Validate a branch that we parse when running cherry_picker --continue 
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)
  • Loading branch information
@abadger
abadger committed Jul 9, 2018
commit 113eb2131931013ee1ec3b3a282f109ce7b37f69
34 changes: 28 additions & 6 deletions 34 cherry_picker/cherry_picker/cherry_picker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,12 +74,7 @@ def upstream(self):

@property
def sorted_branches(self):
def version_from_branch(branch):
try:
return tuple(map(int, re.match(r'^.*(?P<version>\d+(\.\d+)+).*$', branch).groupdict()['version'].split('.')))
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"""
Copy link
Contributor

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.

return sorted(
self.branches,
reverse=True,
Expand Down Expand Up @@ -423,9 +418,36 @@ def get_base_branch(cherry_pick_branch):
return '2.7' from 'backport-sha-2.7'
"""
prefix, sha, base_branch = cherry_pick_branch.split('-', 2)

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):
Copy link
Contributor

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

raise ValueError(f'branch name has an invalid sha: {sha}')

cmd = ['git', 'log', '-r', sha]
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good

try:
subprocess.check_output(cmd, stderr=subprocess.STDOUT)
except subprocess.SubprocessError:
raise ValueError(f'The sha listed in the branch name, {sha}, is not present in the repository')

# Subject the parsed base_branch to the same tests as when we generated it
# This throws a ValueError if the base_branch doesn't need our requirements
version_from_branch(base_branch)

return base_branch


def version_from_branch(branch):
"""
return version information from a git branch name
"""
try:
return tuple(map(int, re.match(r'^.*(?P<version>\d+(\.\d+)+).*$', branch).groupdict()['version'].split('.')))
except AttributeError as attr_err:
raise ValueError(f'Branch {branch} seems to not have a version in its name.') from attr_err


def get_current_branch():
"""
Return the current branch
Expand Down
27 changes: 22 additions & 5 deletions 27 cherry_picker/cherry_picker/test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,19 +32,36 @@ def changedir(d):
os.chdir(cwd)


def test_get_base_branch():
# The format of cherry-pick branches we create are "backport-{SHA}-{base_branch}"
cherry_pick_branch = 'backport-afc23f4-2.7'
@mock.patch('subprocess.check_output')
def test_get_base_branch(subprocess_check_output):
# The format of cherry-pick branches we create are::
# backport-{SHA}-{base_branch}
subprocess_check_output.return_value = b'22a594a0047d7706537ff2ac676cdc0f1dcb329c'
cherry_pick_branch = 'backport-22a594a-2.7'
result = get_base_branch(cherry_pick_branch)
assert result == '2.7'


def test_get_base_branch_which_has_dashes():
cherry_pick_branch ='backport-afc23f4-baseprefix-2.7-basesuffix'
@mock.patch('subprocess.check_output')
def test_get_base_branch_which_has_dashes(subprocess_check_output):
subprocess_check_output.return_value = b'22a594a0047d7706537ff2ac676cdc0f1dcb329c'
cherry_pick_branch = 'backport-22a594a-baseprefix-2.7-basesuffix'
result = get_base_branch(cherry_pick_branch)
assert result == 'baseprefix-2.7-basesuffix'


@pytest.mark.parametrize('cherry_pick_branch', ['backport-22a594a', # Not enough fields
'prefix-22a594a-2.7', # Not the prefix we were expecting
'backport-22a594a-base', # No version info in the base branch
]
)
@mock.patch('subprocess.check_output')
def test_get_base_branch_invalid(subprocess_check_output, cherry_pick_branch):
subprocess_check_output.return_value = b'22a594a0047d7706537ff2ac676cdc0f1dcb329c'
with pytest.raises(ValueError):
get_base_branch(cherry_pick_branch)


@mock.patch('subprocess.check_output')
def test_get_current_branch(subprocess_check_output):
subprocess_check_output.return_value = b'master'
Expand Down
Morty Proxy This is a proxified and sanitized view of the page, visit original site.