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

When everywhere #3340

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

When everywhere #3340

wants to merge 4 commits into from

Conversation

lukaszachy
Copy link
Collaborator

@lukaszachy lukaszachy commented Nov 4, 2024

Pull Request Checklist

  • implement the feature
  • write the documentation
  • extend the test coverage
  • update the specification
  • adjust plugin docstring
  • modify the json schema
  • mention the version
  • include a release note

@lukaszachy lukaszachy linked an issue Nov 4, 2024 that may be closed by this pull request
@lukaszachy lukaszachy added the ci | full test Pull request is ready for the full test execution label Nov 4, 2024
Use case: enable particular step configuration only on some
environments
@lukaszachy lukaszachy marked this pull request as ready for review November 5, 2024 10:40
@lukaszachy lukaszachy added this to the 1.39 milestone Nov 5, 2024
@lukaszachy lukaszachy added specification Metadata specification (core, tests, plans, stories) area | context The context adjust implementation labels Nov 5, 2024
Comment on lines 15 to 16
It is easier limit when step configuration is run by
using :ref:`when-config` key.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
It is easier limit when step configuration is run by
using :ref:`when-config` key.
It is easier to limit a step execution using
the :ref:when-config key.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


prepare:
- name: Prepare config to run only on Fedora
when: distro == fedora
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lukaszachy can we add an example for multiple when rules?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in this way, it soon becomes difficult to read.

Using the ``when`` key makes it easier to restrict a step configuration
to run only if any of the specified rules matches.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this means I can also use something like

when:
  - distro == fedora && trigger = rhel-ci
  - distro == centos && trigger = fedora-ci

?

Copy link
Collaborator

@thrix thrix Nov 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would appreciate more examples here, to get the feel of what can be done here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

example: |
finish:
- name: finish config to run only on Fedora
when: distro == fedora
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lukaszachy could we add a test with multiple when rules maybe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will change tests/steps/when/data/plan.fmf

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tmt/steps/__init__.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@martinhoyer martinhoyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!
I'm thinking if it would make sense if Phase would have a more generic enabled property, but that's just a random thought, no opinion.

docs/guide.rst Outdated
- name: Prepare config to run only on Fedora
when: distro == fedora
how: shell
script: ./fedora_specific.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
script: ./fedora_specific.
script: ./fedora_specific.sh

I assume? or no dot? Same below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

summary: Conditional step configuration
description: |
Using the ``when`` key makes it easier to restrict a step configuration
to run only if any of the specified rules matches.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

run only if any of the specified rules matches.

I wonder if we could better communicate to the user how does that look, perhaps an example like:

If you want mach all conditions(see fmf context documentation), use:

when: "distro == fedora and arch = x86_64"

If you want to mach at least one use:

when: "distro == fedora"
when: "arch = x86_64"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, nice idea but that is common for whole adjust madness so ... another PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO context deserves whole section in https://tmt.readthedocs.io/en/stable/examples.html#

@happz
Copy link
Collaborator

happz commented Nov 7, 2024

Nice! I'm thinking if it would make sense if Phase would have a more generic enabled property, but that's just a random thought, no opinion.

@martinhoyer there's definitely one situation when "enabled" needs an input, i.e. "Is this phase enabled on this guest?", and because of that I think we cannot reach a singular "Is this phase enabled?" property.

@psss psss removed this from the 1.39 milestone Nov 21, 2024
@psss psss added this to the 1.40 milestone Nov 21, 2024
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 ci | full test Pull request is ready for the full test execution specification Metadata specification (core, tests, plans, stories)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow when key for step configs
6 participants