From 3206fd68874fcfdc37f74faa4809f684c69fbe68 Mon Sep 17 00:00:00 2001 From: Laura Barcziova Date: Mon, 27 May 2024 11:57:11 +0200 Subject: [PATCH] Allow passing versions as list for sync_release Fixes packit/packit-service#2408 --- packit/api.py | 42 +++++++++++------- packit/cli/propose_downstream.py | 6 +-- tests/integration/test_source_git.py | 12 ++--- tests/integration/test_source_git_status.py | 4 +- tests/integration/test_update.py | 49 +++++++++++++++------ tests/integration/test_using_cockpit.py | 4 +- tests/unit/test_api.py | 27 +++++++----- tests_recording/test_api.py | 6 +-- 8 files changed, 93 insertions(+), 57 deletions(-) diff --git a/packit/api.py b/packit/api.py index 8ff0b626e..90145e454 100644 --- a/packit/api.py +++ b/packit/api.py @@ -747,7 +747,7 @@ def check_version_distance( current, proposed, target_branch, - ) -> None: + ) -> bool: """Following this guidelines: https://docs.fedoraproject.org/en-US/fesco/Updates_Policy/#philosophy we want to avoid major updates of packages within a **stable** release. @@ -771,14 +771,12 @@ def check_version_distance( and masked_proposed and masked_current.group(0) != masked_proposed.group(0) ): - raise ReleaseSkippedPackitException( - f"The upstream released version {current} does not match " - f"specfile version {proposed} at branch {target_branch} " - f"using the version_update_mask " - f'"{self.package_config.version_update_mask}".' - "\nYou can change the version_update_mask with an empty string " - "to skip this check.", + logger.debug( + f"Masked {current} and {proposed} ({masked_current} and {masked_proposed}) " + f"do not match.", ) + return False + return True @staticmethod def get_upstream_release_monitoring_bug( @@ -830,7 +828,7 @@ def get_upstream_release_monitoring_bug( def sync_release( self, dist_git_branch: Optional[str] = None, - version: Optional[str] = None, + versions: Optional[list[str]] = None, tag: Optional[str] = None, use_local_content=False, add_new_sources=True, @@ -857,7 +855,7 @@ def sync_release( def sync_release( self, dist_git_branch: Optional[str] = None, - version: Optional[str] = None, + versions: Optional[list[str]] = None, tag: Optional[str] = None, use_local_content=False, add_new_sources=True, @@ -883,7 +881,7 @@ def sync_release( def sync_release( self, dist_git_branch: Optional[str] = None, - version: Optional[str] = None, + versions: Optional[list[str]] = None, tag: Optional[str] = None, use_local_content=False, add_new_sources=True, @@ -910,7 +908,7 @@ def sync_release( Args: dist_git_branch: Branch in dist-git, defaults to repo's default branch. use_local_content: Don't check out anything. - version: Upstream version to update in dist-git. + versions: List of new upstream versions. tag: Upstream git tag. add_new_sources: Download and upload source archives. force_new_sources: Download/upload the archive even if it's @@ -952,13 +950,15 @@ def sync_release( dist_git_branch = ( dist_git_branch or self.dg.local_project.git_project.default_branch ) - # process version and tag parameters - if version and tag: + version = None + # process versions and tag parameters + if versions and tag: raise PackitException( - "Function parameters version and tag are mutually exclusive.", + "Function parameters versions and tag are mutually exclusive.", ) if not tag: - version = version or self.up.get_latest_released_version() + # for now let's just pick the first one + version = versions[0] if versions else self.up.get_latest_released_version() if not version: raise PackitException( "Could not figure out version of latest upstream release. " @@ -1054,7 +1054,15 @@ def sync_release( if compare_versions(version, spec_ver) > 0: logger.warning(f"Version {spec_ver!r} in spec file is outdated.") - self.check_version_distance(version, spec_ver, dist_git_branch) + if not self.check_version_distance(version, spec_ver, dist_git_branch): + raise ReleaseSkippedPackitException( + f"The upstream released version {version} does not match " + f"specfile version {spec_ver} at branch {dist_git_branch} " + f"using the version_update_mask " + f'"{self.package_config.version_update_mask}".' + "\nYou can change the version_update_mask with an empty string " + "to skip this check.", + ) self.dg.check_last_commit() diff --git a/packit/cli/propose_downstream.py b/packit/cli/propose_downstream.py index 0f143ca53..0e48fd473 100644 --- a/packit/cli/propose_downstream.py +++ b/packit/cli/propose_downstream.py @@ -76,7 +76,7 @@ def sync_release( api.sync_release( dist_git_branch=branch, use_local_content=local_content, - version=version, + versions=[version], force_new_sources=force_new_sources, upstream_ref=upstream_ref, create_pr=pr, @@ -202,7 +202,7 @@ def propose_downstream( force_new_sources=force_new_sources, pr=pr, path_or_url=path_or_url, - version=version, + versions=[version], force=force, local_content=local_content, upstream_ref=upstream_ref, @@ -247,7 +247,7 @@ def pull_from_upstream( force_new_sources=force_new_sources, pr=pr, path_or_url=path_or_url, - version=version, + versions=[version], force=force, local_content=False, upstream_ref=None, diff --git a/tests/integration/test_source_git.py b/tests/integration/test_source_git.py index c205498cf..0f87c07d9 100644 --- a/tests/integration/test_source_git.py +++ b/tests/integration/test_source_git.py @@ -108,7 +108,7 @@ def test_basic_local_update_without_patching( api_instance_source_git.sync_release( dist_git_branch="main", - version="0.1.0", + versions=["0.1.0"], upstream_ref="0.1.0", mark_commit_origin=True, ) @@ -137,7 +137,7 @@ def test_basic_local_update_empty_patch( mock_spec_download_remote_s(distgit) api_instance_source_git.sync_release( dist_git_branch="main", - version="0.1.0", + versions=["0.1.0"], upstream_ref=ref, ) @@ -176,7 +176,7 @@ def test_basic_local_update_patch_content( api_instance_source_git.sync_release( dist_git_branch="main", - version="0.1.0", + versions=["0.1.0"], upstream_ref="0.1.0", mark_commit_origin=False, ) @@ -322,7 +322,7 @@ def test_basic_local_update_patch_content_with_metadata( api_instance_source_git.sync_release( dist_git_branch="main", - version="0.1.0", + versions=["0.1.0"], upstream_ref="0.1.0", ) @@ -371,7 +371,7 @@ def test_basic_local_update_patch_content_with_metadata_and_patch_ignored( api_instance_source_git.sync_release( dist_git_branch="main", - version="0.1.0", + versions=["0.1.0"], upstream_ref="0.1.0", ) @@ -411,7 +411,7 @@ def test_basic_local_update_patch_content_with_downstream_patch( api_instance_source_git.sync_release( dist_git_branch="main", - version="0.1.0", + versions=["0.1.0"], upstream_ref="0.1.0", ) diff --git a/tests/integration/test_source_git_status.py b/tests/integration/test_source_git_status.py index 4781f8fe5..78845b451 100644 --- a/tests/integration/test_source_git_status.py +++ b/tests/integration/test_source_git_status.py @@ -24,7 +24,7 @@ def check_ready_api_dg_first( mock_spec_download_remote_s(distgit) api_instance_update_source_git.sync_release( dist_git_branch="main", - version="0.1.0", + versions=["0.1.0"], upstream_ref="0.1.0", mark_commit_origin=True, ) @@ -52,7 +52,7 @@ def check_ready_api_sg_first( mock_spec_download_remote_s(distgit) api_instance_update_source_git.sync_release( dist_git_branch="main", - version="0.1.0", + versions=["0.1.0"], upstream_ref="0.1.0", mark_commit_origin=True, ) diff --git a/tests/integration/test_update.py b/tests/integration/test_update.py index 2d96673c1..e34ad07f5 100644 --- a/tests/integration/test_update.py +++ b/tests/integration/test_update.py @@ -52,7 +52,7 @@ def test_basic_local_update( flexmock(api).should_receive("init_kerberos_ticket").at_least().once() flexmock(Specfile).should_call("reload").once() - api.sync_release(dist_git_branch="main", version="0.1.0") + api.sync_release(dist_git_branch="main", versions=["0.1.0"]) assert (d / TARBALL_NAME).is_file() spec = Specfile(d / "beer.spec") @@ -82,7 +82,7 @@ def test_basic_local_update_no_upload_to_lookaside( ).once() api.package_config.upload_sources = False - api.sync_release(dist_git_branch="main", version="0.1.0") + api.sync_release(dist_git_branch="main", versions=["0.1.0"]) assert (d / TARBALL_NAME).is_file() spec = Specfile(d / "beer.spec") @@ -114,7 +114,7 @@ def test_basic_local_update_use_downstream_specfile( api.sync_release( dist_git_branch="main", - version="0.1.0", + versions=["0.1.0"], use_downstream_specfile=True, ) @@ -131,7 +131,7 @@ def test_basic_local_update_use_downstream_specfile( # do this second time to see whether the specfile is updated correctly api.sync_release( dist_git_branch="main", - version="0.1.0", + versions=["0.1.0"], use_downstream_specfile=True, ) @@ -175,7 +175,10 @@ def test_basic_local_update_with_multiple_sources( offline=False, ) - api.sync_release(dist_git_branch="main", version="0.1.0") + api.sync_release( + dist_git_branch="main", + versions=["0.1.0"], + ) assert dist_git_first_source.is_file() assert dist_git_second_source.is_file() @@ -224,7 +227,10 @@ def test_basic_local_update_with_adding_second_source( offline=False, ) - api.sync_release(dist_git_branch="main", version="0.1.0") + api.sync_release( + dist_git_branch="main", + versions=["0.1.0"], + ) assert dist_git_first_source.is_file() assert dist_git_second_source.is_file() @@ -272,7 +278,10 @@ def test_basic_local_update_with_adding_second_local_source( offline=False, ) - api.sync_release(dist_git_branch="main", version="0.1.0") + api.sync_release( + dist_git_branch="main", + versions=["0.1.0"], + ) assert dist_git_first_source.is_file() assert dist_git_second_source.is_file() @@ -318,7 +327,10 @@ def test_basic_local_update_with_adding_second_local_source_tracked_by_git( offline=False, ) - api.sync_release(dist_git_branch="main", version="0.1.0") + api.sync_release( + dist_git_branch="main", + versions=["0.1.0"], + ) assert dist_git_first_source.is_file() assert dist_git_second_source.is_file() @@ -359,7 +371,10 @@ def test_basic_local_update_with_removing_second_source( offline=False, ) - api.sync_release(dist_git_branch="main", version="0.1.0") + api.sync_release( + dist_git_branch="main", + versions=["0.1.0"], + ) assert dist_git_first_source.is_file() assert not dist_git_second_source.is_file() @@ -447,7 +462,10 @@ def test_basic_local_update_copy_upstream_release_description( get_release=lambda name, tag_name: release, ) api.package_config.copy_upstream_release_description = True - api.sync_release(dist_git_branch="main", version="0.1.0") + api.sync_release( + dist_git_branch="main", + versions=["0.1.0"], + ) assert (d / TARBALL_NAME).is_file() spec = Specfile(d / "beer.spec") @@ -477,7 +495,10 @@ def test_basic_local_update_using_distgit( u, d, api = api_instance mock_spec_download_remote_s(d) - api.sync_release(dist_git_branch="main", version="0.1.0") + api.sync_release( + dist_git_branch="main", + versions=["0.1.0"], + ) assert (d / TARBALL_NAME).is_file() spec = Specfile(d / "beer.spec") @@ -511,7 +532,7 @@ def test_basic_local_update_direct_push( _, distgit_remote = distgit_and_remote mock_spec_download_remote_s(d) - api.sync_release(dist_git_branch="main", version="0.1.0", create_pr=False) + api.sync_release(dist_git_branch="main", versions=["0.1.0"], create_pr=False) remote_dir_clone = Path(f"{distgit_remote}-clone") subprocess.check_call( @@ -538,7 +559,7 @@ def test_update_downstream_changelog_even_if_has_autochangelog( api.package_config.sync_changelog = True api.sync_release( dist_git_branch="main", - version="0.1.0", + versions=["0.1.0"], create_pr=False, add_new_sources=False, ) @@ -566,7 +587,7 @@ def test_basic_local_update_direct_push_no_dg_spec( _, distgit_remote = distgit_and_remote mock_spec_download_remote_s(d) - api.sync_release(dist_git_branch="main", version="0.1.0", create_pr=False) + api.sync_release(dist_git_branch="main", versions=["0.1.0"], create_pr=False) remote_dir_clone = Path(f"{distgit_remote}-clone") subprocess.check_call( diff --git a/tests/integration/test_using_cockpit.py b/tests/integration/test_using_cockpit.py index 0bb9ca8d4..5d179016a 100644 --- a/tests/integration/test_using_cockpit.py +++ b/tests/integration/test_using_cockpit.py @@ -77,7 +77,7 @@ def mocked_new_sources(sources=None): api.sync_release( dist_git_branch="main", use_local_content=False, - version="179", + versions=["179"], force_new_sources=False, create_pr=True, ) @@ -119,7 +119,7 @@ def mocked_new_sources(sources=None): assert pr == api.sync_release( dist_git_branch="main", use_local_content=False, - version="179", + versions=["179"], force_new_sources=False, create_pr=True, ) diff --git a/tests/unit/test_api.py b/tests/unit/test_api.py index e11319bc6..2787afc89 100644 --- a/tests/unit/test_api.py +++ b/tests/unit/test_api.py @@ -172,8 +172,9 @@ def test_sync_release_version_tag_processing( ) api_mock.should_receive("push_and_create_pr").and_return(flexmock()) flexmock(PatchGenerator).should_receive("undo_identical") + versions = [version] if version else [] with expectation: - api_mock.sync_release(version=version, tag=tag, dist_git_branch="_") + api_mock.sync_release(versions=versions, tag=tag, dist_git_branch="_") def test_sync_release_do_not_create_sync_note(api_mock): @@ -186,7 +187,7 @@ def test_sync_release_do_not_create_sync_note(api_mock): api_mock.up.package_config.should_receive("get_package_names_as_env").and_return({}) api_mock.up.package_config.create_sync_note = False api_mock.should_receive("push_and_create_pr").and_return(flexmock()) - api_mock.sync_release(version="1.1", dist_git_branch="_") + api_mock.sync_release(versions=["1.1"], dist_git_branch="_") def test_sync_release_create_sync_note(api_mock): @@ -198,7 +199,7 @@ def test_sync_release_create_sync_note(api_mock): ) api_mock.up.package_config.should_receive("get_package_names_as_env").and_return({}) api_mock.should_receive("push_and_create_pr").and_return(flexmock()) - api_mock.sync_release(version="1.1", dist_git_branch="_") + api_mock.sync_release(versions=["1.1"], dist_git_branch="_") def test_sync_release_env(api_mock): @@ -286,7 +287,7 @@ def test_sync_release_sync_files_call(config_mock, upstream_mock, distgit_mock): ], ) - api.sync_release(version="1.1", dist_git_branch="_") + api.sync_release(versions=["1.1"], dist_git_branch="_") def test_sync_release_check_pr_instructions(api_mock): @@ -337,7 +338,11 @@ def test_sync_release_check_pr_instructions(api_mock): repo=DistGit, sync_acls=False, ).and_return(flexmock()) - api_mock.sync_release(version="1.1", dist_git_branch="_", add_pr_instructions=True) + api_mock.sync_release( + versions=["1.1"], + dist_git_branch="_", + add_pr_instructions=True, + ) def test_sync_release_downgrade(api_mock): @@ -350,7 +355,7 @@ def test_sync_release_downgrade(api_mock): api_mock.dg.should_receive("get_specfile_version").and_return("1.1") with pytest.raises(ReleaseSkippedPackitException): api_mock.sync_release( - version="1.0", + versions=["1.0"], dist_git_branch="_", add_pr_instructions=True, ) @@ -506,7 +511,7 @@ def test_pkg_tool_property(package_config, config, expected_pkg_tool): "4.0.0", "rawhide", None, - does_not_raise(), + True, id="skip version distance check for rawhide", ), pytest.param( @@ -514,7 +519,7 @@ def test_pkg_tool_property(package_config, config, expected_pkg_tool): "4.0.0", "f38", r"\d+\.\d+\.", - pytest.raises(PackitException), + False, id="proposed version far too distant for f38", ), pytest.param( @@ -522,7 +527,7 @@ def test_pkg_tool_property(package_config, config, expected_pkg_tool): "3.10.1", "f38", r"\d+\.\d+\.", - does_not_raise(), + True, id="proposed version ok for f38", ), ), @@ -540,12 +545,14 @@ def test_check_version_distance( ) config = Config() - with exp: + assert ( PackitAPI(config, package_config).check_version_distance( current_version, proposed_version, target_branch, ) + == exp + ) @pytest.mark.parametrize( diff --git a/tests_recording/test_api.py b/tests_recording/test_api.py index 68eae9237..5f12143bb 100644 --- a/tests_recording/test_api.py +++ b/tests_recording/test_api.py @@ -89,7 +89,7 @@ def check_version_increase(self): f"git tag -a {version_increase} -m 'my version {version_increase}'", shell=True, ) - self.api.sync_release(version="0.8.1", force=True) + self.api.sync_release(versions=["0.8.1"], force=True) # We don't run this test because we haven't been able to regenerate the data for it # and we're not sure what it's supposed to actually test. @@ -107,7 +107,7 @@ def comment_in_spec(self): f"git tag -a {version_increase} -m 'my version {version_increase}'", shell=True, ) - self.api.sync_release(version="0.8.1") + self.api.sync_release(versions=["0.8.1"]) def test_changelog_sync(self): """ @@ -133,6 +133,6 @@ def test_changelog_sync(self): flexmock(Bugzilla).should_receive("query").and_return([]) self.api.package_config.sync_changelog = True - self.api.sync_release(version="1.0.0", use_local_content=True) + self.api.sync_release(versions=["1.0.0"], use_local_content=True) new_downstream_spec_content = self.api.dg.absolute_specfile_path.read_text() assert changed_upstream_spec_content == new_downstream_spec_content