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

Execute must not use discover phases disabled by their when #3493

Merged
merged 2 commits into from
Jan 28, 2025

Conversation

happz
Copy link
Collaborator

@happz happz commented Jan 28, 2025

This seems to be an omission from when when was added: it is evaluated by tmt, not by fmf, disabled phases are still present in the tree of steps and phases. Most places already ignore them, but execute did not. execute runs its single phase with all discover phases in sequence, and must make sure it does not try to run the disabled ones.

Fixes #3492.

Pull Request Checklist

  • implement the feature

@happz happz added bug Something isn't working step | execute Stuff related to the execute step labels Jan 28, 2025
@happz happz added the ci | full test Pull request is ready for the full test execution label Jan 28, 2025
@happz happz force-pushed the execute-shall-not-use-disable-discover branch from 27fe736 to 29fc1e5 Compare January 28, 2025 10:22
Copy link
Collaborator

@lukaszachy lukaszachy left a comment

Choose a reason for hiding this comment

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

Should we add tests?
I verified it fixes the problem

@happz
Copy link
Collaborator Author

happz commented Jan 28, 2025

Should we add tests? I verified it fixes the problem

Yep, it's there now. It took me some time to modify and extend the existing one.

@lukaszachy lukaszachy added this to the 1.42 milestone Jan 28, 2025
@thrix thrix added the status | ready for merge The only missing piece is to do the rebase the current 'main' and let the CI finish. label Jan 28, 2025
@thrix thrix force-pushed the execute-shall-not-use-disable-discover branch from 854da66 to 73cdd3b Compare January 28, 2025 12:57
@thrix thrix enabled auto-merge (squash) January 28, 2025 12:58
Copy link
Collaborator

@psss psss left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix!

@thrix
Copy link
Collaborator

thrix commented Jan 28, 2025

/packit retest-failed

happz added 2 commits January 28, 2025 21:06
This seems to be an omission from when `when` was added: it is evaluated
by tmt, not by fmf, disabled phases are still present in the tree of
steps and phases. Most places already ignore them, but `execute` did
not. `execute` runs its single phase with all `discover` phases in
sequence, and must make sure it does not try to run the disabled ones.

Fixes #3492.
@thrix thrix force-pushed the execute-shall-not-use-disable-discover branch from 73cdd3b to 8af4e4d Compare January 28, 2025 20:06
@thrix thrix disabled auto-merge January 28, 2025 20:06
@thrix thrix merged commit f664a68 into main Jan 28, 2025
3 of 9 checks passed
@thrix thrix deleted the execute-shall-not-use-disable-discover branch January 28, 2025 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ci | full test Pull request is ready for the full test execution status | ready for merge The only missing piece is to do the rebase the current 'main' and let the CI finish. step | execute Stuff related to the execute step
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Execute tries to run tests from 'turned off' discover config
5 participants