Allow configuring the shared artifact repository name - #5135
Conversation
LecrisUT
left a comment
There was a problem hiding this comment.
Note that SHARED_REPO_NAME is used as a default in ArtifactInfo. You would have to pass the value from PrepareArtifactData down to those as well. Also might as well move the SHARED_REPO_NAME up to this level at this point
psss
left a comment
There was a problem hiding this comment.
Congrats for your first pull request, @HabibRh26! 😁 Looks good, two thoughts:
As this is adding a new option, it would be good to include a release note. These are stored under the pending directory:
- https://github.com/teemtee/tmt/tree/main/docs/releases/pending
- see also https://tmt.readthedocs.io/en/stable/contribute.html#checklist
What about extending some of the existing tests to exercise the newly added option? I think the basic test could be a good candidate:
tmt/tests/prepare/artifact/basic/plan.fmf
Lines 9 to 12 in e3f4b1a
| Name of the tmt generated repo containing all artifacts | ||
| that are generated. | ||
| """, |
There was a problem hiding this comment.
| Name of the tmt generated repo containing all artifacts | |
| that are generated. | |
| """, | |
| Name of the tmt generated repo containing all artifacts | |
| that are generated. | |
| """, |
| #: Repository ID this artifact is available from. Used during verification | ||
| #: to confirm the artifact was installed from the expected repository. | ||
| repo_id: str = SHARED_REPO_NAME | ||
| repo_id: str = 'tmt-artifact-shared' |
There was a problem hiding this comment.
Import from the plugin, or, better, let plugin import from this library. At this moment, 'tmt-artifact-shared' string is duplicated, and we want just the single source of truth.
There was a problem hiding this comment.
I initially moved SHARED_REPO_NAME up to __init__.py based on @LecrisUT's suggestion (If I understand it correctly) to move it to the level where PrepareArtifactData owns the configuration. However, that created a string duplication since providers/__init__.py still needs the value for the ArtifactInfo.repo_id default, and importing it back from __init__.py would cause a circular import (__init__.py already imports from providers/__init__.py).
There was a problem hiding this comment.
Reverted to keep the constant in providers/__init__.py as the single source of truth
|
/packit build |
is the output of this command supposed to display here or somewhere else? |
It will kick off the build jobs and subsequent test jobs in the "checks" box below, you might need to scroll around in the box to see what its doing |
|
/packit build |
Release note added in 9866a397, test coverage in 11ba5d0b and log assertion in 01fc4371 |
01fc437 to
8e551d8
Compare
Fixes #5073
Pull Request Checklist