-
Notifications
You must be signed in to change notification settings - Fork 162
set repository priority from plugin vars #4505
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?
Conversation
b84e1b9 to
aff6b44
Compare
7795993 to
d96954e
Compare
The artifact provider priority is now passed through the constructor instead of being hardcoded. This allows providers to use the priority value from TMT_PLUGIN_PREPARE_ARTIFACT_PRIORITY environment variable or --priority option via self.data.priority.
Reorder parameters in ArtifactProvider and all subclasses to have logger as the last parameter, following the convention used elsewhere in the codebase. Update all callers and unit tests accordingly.
- Rename PrepareArtifactData.priority to default_repository_priority - Update CLI option to --default-repository-priority - Rename provider parameter from priority to repository_priority - Update all provider subclasses and tests - Fix metavar to PRIORITY for better documentation display - Add environment variable documentation in help string
d96954e to
45afc9a
Compare
LecrisUT
left a comment
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.
Just one inconsistency spotted
091dc5c to
43bbeb2
Compare
| provide: | ||
| $ref: "/schemas/common#/definitions/one_or_more_strings" | ||
|
|
||
| default_repository_priority: |
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.
Specification keys are always using - to separate words.
| default_repository_priority: | |
| default-repository-priority: |
| help=""" | ||
| Default priority for created artifact repositories. Lower values mean | ||
| higher priority in package managers. Default: 50. Also uses environment | ||
| variable ``TMT_PLUGIN_PREPARE_ARTIFACT_DEFAULT_REPOSITORY_PRIORITY``. |
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.
Please, don't mention the default value, it will be displayed by tmt and rendered into HTML docs automatically. Also, I advise against mentioning the environment variable, because pretty much all CLI options can be specified via environment variables, and this one is not special enough to deserve its own to be mentioned explicitly. See https://tmt--4505.org.readthedocs.build/en/4505/plugins/prepare.html#id1, it's rendered into HTML, and once I'm done with #4472, we can enable display of envvars in CLI help as well.
|
PR name would deserve an update, to better reflect the change to something like "Accept default repository priority for repositories created by prepare/artifact" |
Pull Request Checklist