Skip to content

Conversation

@vaibhavdaren
Copy link
Contributor

@vaibhavdaren vaibhavdaren commented Jan 18, 2026

Pull Request Checklist

  • implement the feature
  • extend the test coverage

@vaibhavdaren vaibhavdaren changed the title priority from plugin vars set repository priority from plugin vars Jan 18, 2026
@vaibhavdaren vaibhavdaren self-assigned this Jan 20, 2026
@vaibhavdaren vaibhavdaren added plugin | artifact Related to the `prepare/artifact` plugin. status | discuss Needs more discussion before closing labels Jan 20, 2026
@vaibhavdaren vaibhavdaren force-pushed the vaibhav-priority-from-plugin-vars branch 2 times, most recently from b84e1b9 to aff6b44 Compare January 20, 2026 15:58
@vaibhavdaren vaibhavdaren removed the status | discuss Needs more discussion before closing label Jan 20, 2026
@vaibhavdaren vaibhavdaren force-pushed the vaibhav-priority-from-plugin-vars branch 2 times, most recently from 7795993 to d96954e Compare January 20, 2026 17:15
@github-project-automation github-project-automation bot moved this to backlog in planning Jan 20, 2026
@vaibhavdaren vaibhavdaren moved this from backlog to review in planning Jan 20, 2026
@vaibhavdaren vaibhavdaren added this to the 1.66 milestone Jan 20, 2026
@vaibhavdaren vaibhavdaren marked this pull request as ready for review January 20, 2026 18:16
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
@vaibhavdaren vaibhavdaren force-pushed the vaibhav-priority-from-plugin-vars branch from d96954e to 45afc9a Compare January 21, 2026 08:47
@happz happz added the status | blocking other work An important pull request, blocking other pull requests or issues label Jan 21, 2026
@vaibhavdaren vaibhavdaren added the ci | full test Pull request is ready for the full test execution label Jan 21, 2026
@vaibhavdaren vaibhavdaren requested a review from LecrisUT January 21, 2026 21:47
Copy link
Contributor

@LecrisUT LecrisUT left a 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

@vaibhavdaren vaibhavdaren force-pushed the vaibhav-priority-from-plugin-vars branch from 091dc5c to 43bbeb2 Compare January 22, 2026 09:58
provide:
$ref: "/schemas/common#/definitions/one_or_more_strings"

default_repository_priority:
Copy link
Contributor

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.

Suggested change
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``.
Copy link
Contributor

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.

@happz
Copy link
Contributor

happz commented Jan 22, 2026

PR name would deserve an update, to better reflect the change to something like "Accept default repository priority for repositories created by prepare/artifact"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci | full test Pull request is ready for the full test execution plugin | artifact Related to the `prepare/artifact` plugin. status | blocking other work An important pull request, blocking other pull requests or issues

Projects

Status: review

Development

Successfully merging this pull request may close these issues.

Make artifact repository priority configurable via environment variable

4 participants