Do not fetch disabled remote plans in `tmt run` by pkhartsk · Pull Request #4749 · teemtee/tmt

pkhartsk · 2026-03-25T17:33:55Z

Pull Request Checklist

gemini-code-assist

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.

LecrisUT · 2026-03-25T17:36:54Z

/packit build

happz · 2026-03-25T18:52:49Z

@pkhartsk one note WRT naming, you seem to be using "imported" and "remote" incorrectly:

"Do not fetch disabled remote plans in tmt run" - actually, whether the remote plan is disabled or not is not relevant to your PR, as it skips import resolution if the importing (or "local") plan is disabled. The title should be something like "Do not resolve importing plan when it is disabled".
/imported/disabled-by-adjust, summary: Imported plan should be disabled by default, rlPhaseStartTest "Imported plan's import should not be resolved if disabled by adjust" - these are describing importing plan rather than the imported one, as it's the importing plan that's examined and skipped when disabled. In that case there is no imported plan at all.

pkhartsk · 2026-03-26T08:01:13Z

My bad @happz , didn't realise the wording mattered.

* "Do not fetch disabled remote plans in tmt run" - actually, whether the remote plan is disabled or not is not relevant to your PR, as it skips import resolution if the **importing** (or "local") plan is disabled. The title should be something like "Do not resolve importing plan when it is disabled".

Will change.

* `/imported/disabled-by-adjust`, `summary: Imported plan should be disabled by default`,  `rlPhaseStartTest "Imported plan's import should not be resolved if disabled by adjust"` - these are describing **importing** plan rather than the imported one, as it's the importing plan that's examined and skipped when disabled. In that case there is no imported plan at all.

Would it be better to name it /disabled-by-adjust, "Local plan disabled by adjust should not import remote plan", and "Remote plan should not be fetched if disabled by adjust"?

LecrisUT · 2026-03-26T10:30:28Z

Would it be better to name it /disabled-by-adjust, "Local plan disabled by adjust should not import remote plan", and "Remote plan should not be fetched if disabled by adjust"?

Oh indeed it seems identical up to the difference of having adjust considered or not. I would say it should be split into /disabled/default and /disabled/by-adjust.

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:

tmt plans ls
tmt plans ls with --shallow or --import-before-name-filter
tmt plans show
tmt run

and how should the disablement of the parent affect these

Fix teemtee#2968

psss

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?

psss · 2026-04-17T07:57:57Z

+    summary: Local plan disabled by adjust should not import remote plan
+    plan:
+        import:
+            url: https://github.com/teemtee/tmt


What about using here an inaccessible repository? For example:

Suggested change

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.

Still relevant. Can re-use the part in /disabled-internal. I don't remember the naming that we agreed on, but an organization like this could work

/importing-disabled: plan: import: url: https://secret.internal name: /secret/internal/plan /default: enabled: false /by-adjust: enabled: true adjust: enabled: false when: some_context == disable /enabled-by-adjust: enabled: false adjust: enabled: false when: some_context == enable

Thing is that some tests I wrote rely on this one being importable, since I wanna test with the context being the desired value..

Couldn't catch the failure and make sure it fails in an expected way?

Just to reiterate, plan show should just say something like "couldn't fetch" but fail gracefully, right?

Maybe can do that in a different PR. I expect it to be quite tangential to everything else here, and would probably involve how we handle --shallow there as well. Handling ls could be either way, but would be important to be handled consistently to not disrupt testing-farm.

psss · 2026-04-17T08:01:05Z

+                    # 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",


Suggested change

f"Plan {plan.name} is not enabled, skipping imports resolution",

f"Plan '{plan.name}' is not enabled, skipping plan import during tmt run.",

This code now also runs during other commands then run, therefore "during tmt run" is no longer adequate.

psss · 2026-04-20T12:45:27Z

            for plan in unresolved_plans:
                try:
+                    # Do not resolve disabled plans during `tmt run`
+                    if run is not None and not plan.enabled:


Seems that in order to fix the tmt plan ls --enabled use case as well, the following would be sufficient?

Suggested change

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.

Put something akin to this in the latest commit

for plans to give granularity in deciding which plans should and should not run/show/ls

LecrisUT · 2026-04-21T12:56:09Z

+    rlPhaseEnd
+
+
+    rlPhaseStartTest "Remote plan should not be fetched if disabled by adjust"


Oh boy, what happened with all of the diff in this test?

Right, removed everything and added mine, will bring them back

LecrisUT

Overall LGTM, just a style change, and the "what happened with the other tests"

LecrisUT · 2026-04-21T13:04:19Z

+                if not resolve_disabled and not plan.enabled:
+                    plan.debug(
+                        f"Plan '{plan.name}' is not enabled, skipping imports resolution",
+                        level=3,


Suggested change

level=3,

Let's make it more visible on the first debug level

LecrisUT · 2026-04-21T13:06:34Z

        links: Optional[list['LinkNeedle']] = None,
        excludes: Optional[list[str]] = None,
        apply_command_line: bool = True,
+        resolve_disabled: bool = True,


There's quite a bit of double-triple negation here? How about enabled_only?

LecrisUT · 2026-04-21T13:11:44Z

    logger = context.obj.logger.clone().apply_verbosity_options(**kwargs)

-    for plan in context.obj.tree.plans(logger=logger):
+    for plan in context.obj.tree.plans(logger=logger, resolve_disabled=True):


I think plans_ls would need a similar specification. Let's be more explicit for now to make sure we catch the correct behavior. Would be in favor of inverting the default since it is more conscious consideration how one would handle plans that are disabled.

thrix · 2026-04-21T17:24:00Z

+
+
+    rlPhaseStartTest "Remote plan should not be fetched if disabled by adjust"
+        rm -rf '~/.cache/fmf/https:__github.com_teemtee_tmt'


this will not expand correctly with the sundle quotes, will it?

gemini-code-assist Bot reviewed Mar 25, 2026

View reviewed changes

LecrisUT self-assigned this Mar 25, 2026

pkhartsk force-pushed the fix-2968 branch from 5afd9ce to 0ab228d Compare March 25, 2026 17:42

pkhartsk force-pushed the fix-2968 branch from 0ab228d to a7a2349 Compare March 26, 2026 08:11

LecrisUT mentioned this pull request Mar 26, 2026

When should disabled plan.import try to fetch #4757

Open

LecrisUT reviewed Mar 26, 2026

View reviewed changes

Comment thread tests/plan/import/basic.sh

Do not resolve importing plan when it is disabled in tmt run

43a337d

Fix teemtee#2968

thrix added the area | plan import Support for importing remote plans label Apr 1, 2026

thrix added this to planning Apr 1, 2026

github-project-automation Bot moved this to backlog in planning Apr 1, 2026

thrix moved this from backlog to review in planning Apr 1, 2026

thrix assigned psss and happz Apr 1, 2026

happz added the ci | full test Pull request is ready for the full test execution label Apr 14, 2026

happz reviewed Apr 14, 2026

View reviewed changes

Comment thread tmt/base/core.py Outdated

psss requested changes Apr 17, 2026

View reviewed changes

psss added this to the 1.72 milestone Apr 20, 2026

psss reviewed Apr 20, 2026

View reviewed changes

Add resolve_disabled parameter

ec22d06

for plans to give granularity in deciding which plans should and should not run/show/ls

pkhartsk force-pushed the fix-2968 branch from a7a2349 to ec22d06 Compare April 21, 2026 11:20

LecrisUT reviewed Apr 21, 2026

View reviewed changes

thrix reviewed Apr 21, 2026

View reviewed changes

	url: https://github.com/teemtee/tmt
	url: https://localhost/invalid

	f"Plan {plan.name} is not enabled, skipping imports resolution",
	f"Plan '{plan.name}' is not enabled, skipping plan import during tmt run.",

	if run is not None and not plan.enabled:
	if (run is not None or Plan._opt('enabled')) and not plan.enabled:

		rlPhaseEnd


		rlPhaseStartTest "Remote plan should not be fetched if disabled by adjust"



		rlPhaseStartTest "Remote plan should not be fetched if disabled by adjust"
		rm -rf '~/.cache/fmf/https:__github.com_teemtee_tmt'

Conversation

pkhartsk commented Mar 25, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

LecrisUT commented Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

happz commented Mar 25, 2026

Uh oh!

pkhartsk commented Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LecrisUT commented Mar 26, 2026

Uh oh!

Uh oh!

Uh oh!

psss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LecrisUT left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

LecrisUT commented Mar 25, 2026 •

edited

Loading

pkhartsk commented Mar 26, 2026 •

edited

Loading