PR#416: Revert "Support for epel*-playground branch requests" - fedpkg

fedpkg

#416 Revert "Support for epel*-playground branch requests"

Merged 3 years ago by onosek. Opened 3 years ago by salimma.

salimma/fedpkg no-epel-playground into master

Fix tests

Michel Alexandre Salim • 3 years ago

d32cb76

Revert "Support for epel*-playground branch requests"

Michel Alexandre Salim • 3 years ago

d8b50b3

fedpkg/cli.py

file modified

+7 -23

		`@@ -1015,26 +1015,10 @@`
		`'a git repository')`

		`pdc_url = config.get('{0}.pdc'.format(name), 'url')`
		`- # When a 'epel\d' branch is requested, it should automatically request`
		`- # 'epel\d+-playground' branch.`
		`- epel_playground = False`
		`- epel_version = None`
		`if branch:`
		`- # Check if the requested branch is an epel branch`
		`- match = re.match(r'^epel(?P<epel_version>\d+)$', branch)`
		`- if match:`
		`- epel_playground = True`
		`- epel_version = int(match.groupdict()["epel_version"])`
		`-`
		`if is_epel(branch):`
		`assert_valid_epel_package(repo_name, branch)`

		`- # Requesting epel\d-playground branches is not allowed`
		`- if bool(re.match(r'^epel\d+-playground$', branch)):`
		`- raise rpkgError(`
		`- 'You cannot directly request {0} branch, as they are '`
		`- 'created as part of epel branch requests'.format(branch))`
		`-`
		`if ns in ['modules', 'test-modules', 'flatpaks']:`
		`branch_valid = bool(re.match(r'^[a-zA-Z0-9.\-_+]+$', branch))`
		`if not branch_valid:`
		`@@ -1044,7 +1028,13 @@`
		`'names'.format('flatpak' if ns == 'flatpaks' else 'module'))`
		`release_branches = list(itertools.chain(`
		`*list(get_release_branches(pdc_url).values())))`
		`- if branch in release_branches:`
		`+ # treat epel-playground the same as epel release branches`
		`+ playground_match = re.match(r'^(epel\d+)-playground$', branch)`
		`+ if playground_match:`
		`+ _branch = playground_match.groups()[0]`
		`+ else:`
		`+ _branch = branch`
		`+ if _branch in release_branches:`
		`if service_levels:`
		`raise rpkgError(`
		`'You can\'t provide SLs for release branches')`
		`@@ -1069,11 +1059,6 @@`
		`*list(get_release_branches(pdc_url).values())))`
		`branches = [b for b in release_branches`
		`if re.match(r'^(f\d+)$', b)]`
		`- # If the requested branch is epel branch then also add epel\d+-playground branch`
		`- # to the request list.`
		`- # TODO: Remove the check for epel version >= 7 when we enable playground for epel7`
		`- elif epel_playground and epel_version >= 8:`
		`- branches = [branch, branch+"-playground"]`
		`else:`
		`branches = [branch]`

		`@@ -1102,7 +1087,6 @@`
		`auto_module = (`
		`ns == 'rpms'`
		`and not re.match(RELEASE_BRANCH_REGEX, b)`
		`- and not epel_playground # Dont run auto_module on epel requests`
		`and not no_auto_module`
		`)`
		`if auto_module:`

test/test_cli.py

file modified

+44 -30

		`@@ -966,11 +966,7 @@`
		`def test_request_epel_branch_override(`
		`self, mock_grb, mock_request_post, mock_request_get`
		`):`
		`- """Tests request-epel-branch-override`
		`-`
		`- epel8 branch operation generates two requests for two branches:`
		`- epel8-playground and epel8 itself`
		`- """`
		`+ """Tests request-epel-branch-override"""`
		`mock_grb.return_value = {'fedora': ['f25', 'f26', 'f27'],`
		`'epel': ['el6', 'epel7', 'epel8']}`
		`mock_rv = Mock()`
		`@@ -983,7 +979,7 @@`
		`mock_rv.json.return_value = {"arches": [], "packages": {}}`
		`mock_request_get.return_value = mock_rv`
		`# Checkout the epel7 branch`
		`- self.run_cmd(['git', 'checkout', 'epel8'], cwd=self.cloned_repo_path)`
		`+ self.run_cmd(['git', 'checkout', 'epel7'], cwd=self.cloned_repo_path)`

		`cli_cmd = ['fedpkg-stage', '--path', self.cloned_repo_path,`
		`'request-branch', '--repo', 'sudoku', 'epel8']`
		`@@ -994,53 +990,71 @@`
		`'action': 'new_branch',`
		`'repo': 'sudoku',`
		`'namespace': 'rpms',`
		`- 'branch': None, # to be changed during testing`
		`+ 'branch': 'epel8',`
		`'create_git_branch': True`
		`}`
		`- self.assertEqual(len(mock_request_post.call_args_list), 2)`
		`+ self.assertEqual(len(mock_request_post.call_args_list), 1)`

		`- # First record`
		`# Get the data that was submitted to Pagure`
		`post_data = mock_request_post.call_args_list[0][1]['data']`
		`actual_issue_content = json.loads(json.loads(`
		post_data)['issue_content'].strip('```'))
		`- expected_issue_content['branch'] = 'epel8-playground'`
		`- self.assertEqual(expected_issue_content, actual_issue_content)`
		`-`
		`- # Second record`
		`- post_data = mock_request_post.call_args_list[1][1]['data']`
		`- actual_issue_content = json.loads(json.loads(`
		- post_data)['issue_content'].strip('```'))
		`- expected_issue_content['branch'] = 'epel8'`
		`self.assertEqual(expected_issue_content, actual_issue_content)`

		`output = sys.stdout.getvalue().strip()`
		`- # 2 same addresses separated with end of line`
		`- expected_output = ('\n'.join(2 * ['https://pagure.stg.example.com/releng/'`
		`- 'fedora-scm-requests/issue/2']))`
		`+ expected_output = ('https://pagure.stg.example.com/releng/'`
		`+ 'fedora-scm-requests/issue/2')`
		`self.assertEqual(output, expected_output)`

		`+ @patch('requests.get')`
		`@patch('requests.post')`
		`@patch('fedpkg.cli.get_release_branches')`
		`@patch('sys.stdout', new=StringIO())`
		`- def test_request_epel_playground_branch_override(self, mock_grb, mock_request_post):`
		`- """Tests request-epel-playground-branch-override`
		`-`
		`- Can not request this branch directly`
		`- """`
		`+ def test_request_epel_playground_branch_override(`
		`+ self, mock_grb, mock_request_post, mock_request_get`
		`+ ):`
		`+ """Tests request-epel-branch-override"""`
		`mock_grb.return_value = {'fedora': ['f25', 'f26', 'f27'],`
		`- 'epel': ['el6', 'epel7']}`
		`+ 'epel': ['el6', 'epel7', 'epel8']}`
		`mock_rv = Mock()`
		`mock_rv.ok = True`
		`mock_rv.json.return_value = {'issue': {'id': 2}}`
		`mock_request_post.return_value = mock_rv`

		`+ mock_rv = Mock()`
		`+ mock_rv.ok = True`
		`+ mock_rv.json.return_value = {"arches": [], "packages": {}}`
		`+ mock_request_get.return_value = mock_rv`
		`+ # Checkout the epel7 branch`
		`+ self.run_cmd(['git', 'checkout', 'epel7'], cwd=self.cloned_repo_path)`
		`+`
		`cli_cmd = ['fedpkg-stage', '--path', self.cloned_repo_path,`
		`- 'request-branch', '--repo', 'sudoku', 'epel7-playground']`
		`+ 'request-branch', '--repo', 'sudoku', 'epel8-playground']`
		`cli = self.get_cli(cli_cmd)`
		`- expected_error = r'^You cannot directly request epel7-playground branch, .*$'`
		`- with six.assertRaisesRegex(self, rpkgError, expected_error):`
		`- cli.request_branch()`
		`+ cli.request_branch()`
		`+`
		`+ expected_issue_content = {`
		`+ 'action': 'new_branch',`
		`+ 'repo': 'sudoku',`
		`+ 'namespace': 'rpms',`
		`+ 'branch': 'epel8-playground',`
		`+ 'create_git_branch': True`
		`+ }`
		`+ #self.assertEqual(len(mock_request_post.call_args_list), 1)`
		`+`
		`+ # Get the data that was submitted to Pagure`
		`+ post_data = mock_request_post.call_args_list[0][1]['data']`
		`+ actual_issue_content = json.loads(json.loads(`
		+ post_data)['issue_content'].strip('```'))
		`+ self.assertEqual(expected_issue_content, actual_issue_content)`
		`+`
		`+ output = sys.stdout.getvalue().strip()`
		`+ # 3 same addresses separated with end of line`
		`+ expected_output = ('\n'.join(3 * ['https://pagure.stg.example.com/releng/'`
		`+ 'fedora-scm-requests/issue/2']))`
		`+ #expected_output = ('https://pagure.stg.example.com/releng/'`
		`+ # 'fedora-scm-requests/issue/2')`
		`+ self.assertEqual(output, expected_output)`

		`@patch('requests.post')`
		`@patch('fedpkg.cli.get_release_branches')`

salimma commented 3 years ago

Per recent changes discussed in the EPEL SC meetings, epel*-playground
branches should no longer be created automatically when a corresponding
epel* branch is requested.

Reverting this commit makes epel* branches behave like normal branches
and no longer disallow epel*-playground branches from being manually
requested if needed.

This reverts commit eabb5ae.

Fixes: #414

Signed-off-by: Michel Alexandre Salim michel@michel-slm.name

1 new commit added

Fix tests

3 years ago

salimma commented 3 years ago

This turns out to be more complex than I thought. Will comment when I have something functional.

onosek commented 3 years ago

I found where the problem is:

  and not re.match(RELEASE_BRANCH_REGEX, b)
- and not epel_playground  # Dont run auto_module on epel requests
  and not no_auto_module

this row needs to be reverted. This check is needed for not to create extra branches for modules.
epel_playground can be renamed to playground_match which is already defined and contains data.

@salimma, I can modify if for you.

Edited 3 years ago by onosek

onosek commented 3 years ago

Commit ff8df26 fixes this pull-request