-
Notifications
You must be signed in to change notification settings - Fork 164
Auto-verify artifact packages against require/recommend #4726
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -370,6 +370,44 @@ def _emit_phase( | |||||||||||||||||||
| missing='skip', | ||||||||||||||||||||
| ) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| # Auto-inject a verify-installation phase when: | ||||||||||||||||||||
| # - at least one artifact phase has verify=True, and | ||||||||||||||||||||
| # - no explicit verify-installation phase was configured by the user. | ||||||||||||||||||||
| from tmt.steps.prepare.artifact import PrepareArtifact | ||||||||||||||||||||
| from tmt.steps.prepare.verify_installation import ( | ||||||||||||||||||||
| PrepareVerifyInstallation, | ||||||||||||||||||||
| PrepareVerifyInstallationData, | ||||||||||||||||||||
| ) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| artifact_phases_with_verify = [ | ||||||||||||||||||||
| phase | ||||||||||||||||||||
| for phase in self._phases | ||||||||||||||||||||
| if isinstance(phase, PrepareArtifact) and phase.data.verify | ||||||||||||||||||||
| ] | ||||||||||||||||||||
| has_verify_phase = any( | ||||||||||||||||||||
| isinstance(phase, PrepareVerifyInstallation) for phase in self._phases | ||||||||||||||||||||
| ) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if artifact_phases_with_verify and not has_verify_phase: | ||||||||||||||||||||
| # Mirror the where= scope of the artifact phases: if any artifact | ||||||||||||||||||||
| # phase runs on all guests (empty where), verify should too; | ||||||||||||||||||||
| # otherwise restrict to the union of their guest/role targets. | ||||||||||||||||||||
| if any(not phase.data.where for phase in artifact_phases_with_verify): | ||||||||||||||||||||
| verify_where: list[str] = [] | ||||||||||||||||||||
| else: | ||||||||||||||||||||
| verify_where = list( | ||||||||||||||||||||
| {dest for phase in artifact_phases_with_verify for dest in phase.data.where} | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| verify_data = PrepareVerifyInstallationData( | ||||||||||||||||||||
| name='verify-artifact-packages', | ||||||||||||||||||||
| how='verify-installation', | ||||||||||||||||||||
| summary='Verify packages were installed from artifact repositories', | ||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||||||||||||||||
| order=tmt.steps.PHASE_ORDER_PREPARE_VERIFY_INSTALLATION, | ||||||||||||||||||||
| auto=True, | ||||||||||||||||||||
| where=verify_where, | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| self._phases.append(PreparePlugin.delegate(self, data=verify_data)) | ||||||||||||||||||||
|
Comment on lines
+391
to
+409
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Uph, please not like this. Would rather it be created inside the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For reference, see Discover dist-git in how it inserts the phase dynamically
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does, but it's a
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Uph. At the minimum, encapsulate it in a function, and document to get rid of it as soon as possible.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On second thought, I still don't like it. What does it take to inject the phase in the queue properly and improperly?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changing how queues are populated in steps, adding API to support dynamic additions while the queue is already running, which most likely requires reordering of the queued tasks as tasks were enqueued according to phases'
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And a unit test, to make sure it works because this would be rather a rare use case. If there's a bug in how queue works, we'd see tmt explode everywhere; if there's a bug in how tasks are enqueued from another tasks of the same step, only a handful of tests would fail, so this calls for having a test as well.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We did create the interface in preparation for it at least Lines 1322 to 1330 in b32fa85
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| # Prepare guests (including workdir sync) | ||||||||||||||||||||
| guest_copies: list[Guest] = [] | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
|
|
||
| import fmf.utils | ||
|
|
||
| import tmt.base.core | ||
| import tmt.steps | ||
| import tmt.utils | ||
| from tmt.container import container, field | ||
|
|
@@ -26,6 +27,18 @@ class PrepareVerifyInstallationData(PrepareStepData): | |
| normalize=tmt.utils.normalize_string_dict, | ||
| ) | ||
|
|
||
| auto: bool = field( | ||
| default=False, | ||
| option='--auto/--no-auto', | ||
| is_flag=True, | ||
| help=""" | ||
| Automatically verify packages listed in ``require`` or ``recommend`` | ||
| that are present in the artifact metadata (``artifacts.yaml``) were | ||
| installed from the artifact repository. Entries in ``verify`` take | ||
| precedence over auto-detected ones. | ||
| """, | ||
| ) | ||
|
|
||
|
|
||
| @tmt.steps.provides_method('verify-installation') | ||
| class PrepareVerifyInstallation(PreparePlugin[PrepareVerifyInstallationData]): | ||
|
|
@@ -58,6 +71,76 @@ class PrepareVerifyInstallation(PreparePlugin[PrepareVerifyInstallationData]): | |
|
|
||
| _data_class = PrepareVerifyInstallationData | ||
|
|
||
| def _build_auto_verify(self, guest: Guest) -> dict[str, str]: | ||
| """ | ||
| Build a verify mapping from artifact metadata and test requirements. | ||
|
|
||
| Reads ``artifacts.yaml`` from the plan workdir, collects all | ||
| package names provided by artifact providers, and intersects them | ||
| with packages listed in ``require``/``recommend`` of tests enabled | ||
| on this guest. Returns a mapping of intersecting package names to | ||
| the artifact shared repository name. | ||
| """ | ||
|
Comment on lines
+74
to
+83
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, the second issue with not being able to inject the phase is having this magic method. Didn't like the branching in discover, and this just adds another flavor of it. This wouldn't even work if you have one artifact provider with verify and another without since it cannot discriminate between them, unless there is even more introspection between these phases |
||
| from tmt.steps.prepare.artifact import ( | ||
| ARTIFACT_METADATA_FILENAME, | ||
| ARTIFACT_SHARED_REPO_NAME, | ||
| ) | ||
|
|
||
| artifacts_file = self.plan_workdir / ARTIFACT_METADATA_FILENAME | ||
|
|
||
| if not artifacts_file.exists(): | ||
| self.debug('No artifacts.yaml found, skipping auto-verification.') | ||
| return {} | ||
|
|
||
| try: | ||
| metadata = tmt.utils.yaml_to_dict(artifacts_file.read_text()) | ||
| except tmt.utils.GeneralError as err: | ||
| self.warn(f"Failed to read artifacts.yaml: {err}") | ||
| return {} | ||
|
|
||
| # Collect all package names provided by artifact providers. | ||
| artifact_package_names: set[str] = { | ||
| artifact['version']['name'] | ||
| for provider in metadata.get('providers', []) | ||
| for artifact in provider.get('artifacts', []) | ||
| } | ||
|
|
||
| if not artifact_package_names: | ||
| self.debug('No artifact packages found in artifacts.yaml.') | ||
| return {} | ||
|
|
||
| # Collect require/recommend package names for tests enabled on this guest. | ||
| # Only DependencySimple entries are package names; fmf/file deps are skipped. | ||
| require_recommend_names: set[str] = set() | ||
| for test_origin in self.step.plan.discover.tests(enabled=True): | ||
| test = test_origin.test | ||
| if not test.enabled_on_guest(guest): | ||
| continue | ||
| for dep in (*test.require, *test.recommend): | ||
| if isinstance(dep, tmt.base.core.DependencySimple): | ||
| require_recommend_names.add(str(dep)) | ||
|
|
||
| intersection = artifact_package_names & require_recommend_names | ||
| if not intersection: | ||
| self.debug('No overlap between artifact packages and test requirements.') | ||
| return {} | ||
|
|
||
| self.debug( | ||
| f"Auto-verifying {fmf.utils.listed(sorted(intersection), 'package')} " | ||
| f"against '{ARTIFACT_SHARED_REPO_NAME}'." | ||
| ) | ||
|
|
||
| unverified = require_recommend_names - artifact_package_names | ||
| if unverified: | ||
| self.warn( | ||
| f"{fmf.utils.listed(sorted(unverified), 'package')} " | ||
| f"from require/recommend not found in artifacts.yaml, " | ||
| f"consider adding a custom verify-installation entry: " | ||
| f"{', '.join(sorted(unverified))}" | ||
| ) | ||
|
|
||
| return dict.fromkeys(intersection, ARTIFACT_SHARED_REPO_NAME) | ||
|
|
||
| def go( | ||
| self, | ||
| *, | ||
|
|
@@ -71,17 +154,24 @@ def go( | |
| if self.is_dry_run: | ||
| return outcome | ||
|
|
||
| if not self.data.verify: | ||
| # Build the effective verify mapping: auto-detected entries are | ||
| # overridden by any manually specified ones. | ||
| effective_verify: dict[str, str] = {} | ||
| if self.data.auto: | ||
| effective_verify.update(self._build_auto_verify(guest)) | ||
| effective_verify.update(self.data.verify) | ||
|
|
||
| if not effective_verify: | ||
| self.verbose('No packages to verify.') | ||
| return outcome | ||
|
|
||
| self.info( | ||
| fmf.utils.listed(list(self.data.verify.keys()), 'package'), | ||
| fmf.utils.listed(list(effective_verify.keys()), 'package'), | ||
| color='green', | ||
| ) | ||
|
|
||
| try: | ||
| package_origins = guest.package_manager.get_package_origin(self.data.verify.keys()) | ||
| package_origins = guest.package_manager.get_package_origin(effective_verify.keys()) | ||
| except (NotImplementedError, tmt.utils.GeneralError) as err: | ||
| error: Exception = ( | ||
| tmt.utils.PrepareError( | ||
|
|
@@ -103,7 +193,7 @@ def go( | |
| return outcome | ||
|
|
||
| failed_packages: list[str] = [] | ||
| for package, expected_repo in self.data.verify.items(): | ||
| for package, expected_repo in effective_verify.items(): | ||
| actual_origin = package_origins[package] | ||
|
|
||
| if actual_origin == expected_repo: | ||
|
|
||
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.
Define
nameas a constant. This improves maintainability by centralizing the string literal, making it easier to update and ensuring consistency if referenced elsewhere.