Skip to content
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

Clarify the trigger context dimension values #2961

Merged
merged 1 commit into from
May 28, 2024
Merged

Conversation

psss
Copy link
Collaborator

@psss psss commented May 27, 2024

Provide a bit more precise definitions of the commit and build trigger values, especially to make it clear that build could be used for the event of promoting draft builds as well.

Related issue: https://issues.redhat.com/browse/OSCI-4586

Pull Request Checklist

  • update the specification

@psss psss added specification Metadata specification (core, tests, plans, stories) area | context The context adjust implementation labels May 27, 2024
@psss psss added this to the 1.34 milestone May 27, 2024
@psss psss added the code | trivial A simple patch - a couple of lines, an easy-to-understand change, a typo fix. label May 27, 2024
@LecrisUT
Copy link
Contributor

Regarding these sections, it would be good to mention who is meant to be implementing these context values. E.g. distro can be a confusing one because after provision, you expect tmt to know what environment it is in, particularly as it is being used to process prepare.install

@happz
Copy link
Collaborator

happz commented May 27, 2024

FWIW, I'm not even sure the list of trigger values belongs in tmt documentation.

"The trigger dimension supports the following values" - well, it supports way more values, right? Any string can be a value of the trigger dimension, depending on whoever runs tmt. If I choose to use a string not listed here, tmt will not crash. The same goes for "Supported context dimensions". The dimensions and values listed in this document have meaning just and only among the CI systems our community controls and manages. For tmt,all these values are opaque, evaluated by fmf, dimensions, and values are taken from fmf files and matched against when conditions, and that's it.

In my world, this would be something that belongs to the document "RH & Fedora CI systems", chapter "These are the dimensions RH & Fedora CI systems set for your tmt plans and tests". It'd be nice to have a big rubber stamp on top of such a document, "tmt developers approve this list, these are the dimensions we think are useful and valuable", but I for one don't see it as something tmt itself should define for every possible user out there.

That said, if I'm not mistaken, we don't have a document like that, therefore I approved the improvement, but it might be something to consider during your documentation focus, @psss? :)

@kkaarreell
Copy link
Collaborator

In my world, this would be something that belongs to the document "RH & Fedora CI systems", chapter "These are the dimensions RH & Fedora CI systems set for your tmt plans and tests". It'd be nice to have a big rubber stamp on top of such a document, "tmt developers approve this list, these are the dimensions we think are useful and valuable", but I for one don't see it as something tmt itself should define for every possible user out there.

Exactly, on tmt side it could be a recommendation at best and it should be implemented and documented on the CI system side.

@psss
Copy link
Collaborator Author

psss commented May 28, 2024

Yeah, these are very good points. Perhaps, on the tmt side we could have just an outline of the meaning, an overal direction how this could/should be used, but the exact definitions should go to the pipe docs, completely agreed.

@psss
Copy link
Collaborator Author

psss commented May 28, 2024

Regarding these sections, it would be good to mention who is meant to be implementing these context values.

I think this is (at least partially) covered at the beginning of the Context section:

The context is usually defined from the command line using the --context option. All dimensions are optional. It is possible to define your own dimensions to describe the context, just make sure the dimension name does not conflict with reserved names.

Each plan can also provide its own context. The context definition provided directly on the command line overrides defined dimensions. All context dimension values are handled in case-insensitive way.

So far there is no auto-detection from the provision step.

@happz happz added status | ready for merge The only missing piece is to do the rebase the current 'main' and let the CI finish. code | no functional change "No Functional Change" intended. Patch should not change tmt's behavior in any way. labels May 28, 2024
Provide a bit more precise definitions of the `commit` and `build`
trigger values, especially to make it clear that `build` could be
used for the event of promoting draft builds as well.
@happz
Copy link
Collaborator

happz commented May 28, 2024

Changes specification and docs only, merging without functional tests.

@happz happz merged commit 63cabb8 into main May 28, 2024
9 of 19 checks passed
@happz happz deleted the trigger-description branch May 28, 2024 12:56
happz pushed a commit that referenced this pull request Jun 2, 2024
Provide a bit more precise definitions of the `commit` and `build`
trigger values, especially to make it clear that `build` could be
used for the event of promoting draft builds as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area | context The context adjust implementation code | no functional change "No Functional Change" intended. Patch should not change tmt's behavior in any way. code | trivial A simple patch - a couple of lines, an easy-to-understand change, a typo fix. specification Metadata specification (core, tests, plans, stories) status | ready for merge The only missing piece is to do the rebase the current 'main' and let the CI finish.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants