List packages from repository providers in artifacts.yaml#4703
List packages from repository providers in artifacts.yaml#4703AthreyVinay wants to merge 8 commits intoavinay-copr-repo-enumeratefrom
artifacts.yaml#4703Conversation
AthreyVinay
added
step | prepare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable refactoring for artifact providers, particularly for repository-based ones. Centralizing Copr-related logic into a base class and shared module is a great improvement for maintainability. The new approach of having providers yield Repository objects for centralized installation is a cleaner design. The core new feature, enumerating packages from repositories to populate artifacts.yaml, is well-implemented. I have one suggestion to improve the robustness and logging in the new enumerate_artifacts method.
AthreyVinay
changed the title
artifacts.yamlartifacts.yaml
AthreyVinay
force-pushed
the
avinay-copr-repo-enumerate
branch
2 times, most recently
from
8014c88 to
d25eaaf
Compare
AthreyVinay
force-pushed
the
avinay-copr-repo-enumerate
branch
from
d25eaaf to
0a48c6b
Compare
AthreyVinay
force-pushed
the
avinay-copr-repo-enumerate
branch
from
0a48c6b to
ea55382
Compare
AthreyVinay
force-pushed
the
avinay-enumerate-artifacts
branch
from
2537914 to
4f186c0
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| nvr_epoch, sep, arch = nevra.rpartition('.') | ||
| if not sep: | ||
| raise ValueError(f"Cannot parse arch from NEVRA '{nevra}'.") | ||
| parts = nvr_epoch.rsplit('-', 2) | ||
| if len(parts) != 3: | ||
| raise ValueError(f"Cannot parse NVR from NEVRA '{nevra}'.") |
There was a problem hiding this comment.
Why not just rsplit(maxsplit) and expand the tuple. There is no expectation of this failing unless a non-nvr is provided.
But also this can be done in a single regex (?P<name>.*)-(?:(?P<epoch>\d+)\:)?(?P<version>.*)-(?P<release>.*)\.(?P<arch>.*). I know we have this regex 1-2 places already even.
There was a problem hiding this comment.
Kept the explicit check to give a useful error message when dnf repoquery emits unexpected non-NEVRA lines. Will change it to a regex match check.
| @abstractmethod | ||
| def artifacts(self) -> Sequence[ArtifactInfo]: |
There was a problem hiding this comment.
Well, can make it non-abstract if you always provide a _artifacts
| self.sanitized_id = tmt.utils.sanitize_name(raw_id, allow_slash=False) | ||
|
|
||
| self.id = self._extract_provider_id(raw_id) | ||
| self._artifacts: list[ArtifactInfo] = [] |
There was a problem hiding this comment.
Add it to the class and document its intent
| def enumerate_artifacts( | ||
| self, guest: Guest | ||
| ) -> None: # TODO: refactor this once the NEVRA parsing is centralized. | ||
| """ | ||
| Enumerate artifacts available from this provider after its repositories | ||
| have been installed on the guest. | ||
| """ |
There was a problem hiding this comment.
This needs a special handling when the artifacts are from tmt-shared-repo, otherwise all providers that write to it would overwrite each other.
There was a problem hiding this comment.
Apologies - do not clearly understand the concern here. Can you please elaborte?
| logger=provider_logger, | ||
| ) | ||
|
|
||
| self._detect_duplicate_nvras(provider, seen_nvras) |
There was a problem hiding this comment.
The position and usage of this is now questionable again
There was a problem hiding this comment.
Yep - this was raised in the design decision. Open to ideas :)
AthreyVinay
force-pushed
the
avinay-copr-repo-enumerate
branch
from
ea55382 to
e076df0
Compare
4 participants
resolves: #4645