Do not fetch disabled remote plans in tmt run#4749
Do not fetch disabled remote plans in tmt run#4749pkhartsk wants to merge 1 commit intoteemtee:mainfrom
tmt run#4749Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a feature to prevent tmt from resolving imports for plans that are disabled by an adjust rule during a tmt run command. The change adds a check in tmt/base/core.py to skip import resolution if a plan is disabled. A new test case and plan definition validate this behavior. No feedback to provide.
|
/packit test |
|
@pkhartsk one note WRT naming, you seem to be using "imported" and "remote" incorrectly:
|
|
My bad @happz , didn't realise the wording mattered.
Will change.
Would it be better to name it |
Oh indeed it seems identical up to the difference of having But looking at the tests done for that one, it doesn't seem to do meaningful checks either. At the minimum I would expect it to check for the git cloning (there should be a specific place where the data is cloned to that can be used for the check). The complications is that we have multiple scenarios that we would need to confirm the git cloning is done or not:
and how should the disablement of the parent affect these |
| rlPhaseStartTest "Remote plan should not be fetched if disabled by adjust" | ||
| rlRun -s "tmt -c distro=fedora-rawhide run --remove --dry plan --name /plans/disabled-by-adjust" 2 "Expect no plans to be found" | ||
| rlRun -s "tmt -c distro=fedora-rawhide -c how=full plan show /plans/disabled-by-adjust" 0 "Show plan correctly" | ||
| rlPhaseEnd |
There was a problem hiding this comment.
You can run these with -dd and check rlAssertNotGrep "Run command: git clone" (we will improve it when switching to pytest). And also the debug message you just added.
Currently when no plans are enabled the output is No plans found. let's also check for that one.
| f"Plan {plan.name} is not enabled, skipping imports resolution", | ||
| level=3, | ||
| ) | ||
| continue |
There was a problem hiding this comment.
This if block does not have to be under the try/except block.
psss
left a comment
psss
left a comment
There was a problem hiding this comment.
Overall the approach seems to be good. Added a couple suggestions. Do we want to cover the tmt plan ls and tmt plan show cases here as well? Or we keep the scope to tmt run only?
| summary: Local plan disabled by adjust should not import remote plan | ||
| plan: | ||
| import: | ||
| url: https://github.com/teemtee/tmt |
There was a problem hiding this comment.
What about using here an inaccessible repository? For example:
| url: https://github.com/teemtee/tmt | |
| url: https://localhost/invalid |
In this way we would clearly/simply verify that an unexpected git clone was not performed. And it would also reveal that the tmt plan show case is actually failing.
| # Do not resolve disabled plans during `tmt run` | ||
| if run is not None and not plan.enabled: | ||
| plan.debug( | ||
| f"Plan {plan.name} is not enabled, skipping imports resolution", |
There was a problem hiding this comment.
| f"Plan {plan.name} is not enabled, skipping imports resolution", | |
| f"Plan '{plan.name}' is not enabled, skipping plan import during tmt run.", |
| f"Plan {plan.name} is not enabled, skipping imports resolution", | ||
| level=3, | ||
| ) | ||
| continue |
| for plan in unresolved_plans: | ||
| try: | ||
| # Do not resolve disabled plans during `tmt run` | ||
| if run is not None and not plan.enabled: |
There was a problem hiding this comment.
Seems that in order to fix the tmt plan ls --enabled use case as well, the following would be sufficient?
| if run is not None and not plan.enabled: | |
| if (run is not None or Plan._opt('enabled')) and not plan.enabled: |
If the fix would be as simple as this (or similar complexity), I'd suggest to fix both use cases in this pull request.
5 participants
Fix #2968
Pull Request Checklist