Skip to content

Do not fetch disabled remote plans in tmt run #4749

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.

Sign up for GitHub

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

Open
pkhartsk wants to merge 1 commit into
base: main
Choose a base branch
from
+22 −0
Open

Do not fetch disabled remote plans in tmt run #4749

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions tests/plan/import/basic.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,11 @@ rlJournalStart
rlRun -s "tmt -c how=full run -r --dry plan -n /plans/full/tmt" 0 "Run plan (dry mode)"
rlPhaseEnd

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
Comment on lines +142 to +145
Copy link
Copy Markdown
Member

@LecrisUT LecrisUT Mar 26, 2026

Choose a reason for hiding this comment

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

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.


rlPhaseStartCleanup
rlRun "popd"
rlPhaseEnd
Expand Down
10 changes: 10 additions & 0 deletions tests/plan/import/data/plans.fmf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,16 @@
name: /plans/provision/connect
enabled: true

/disabled-by-adjust:
summary: Local plan disabled by adjust should not import remote plan
plan:
import:
url: https://github.com/teemtee/tmt
Copy link
Copy Markdown
Member

@psss psss Apr 17, 2026

Choose a reason for hiding this comment

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

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.

name: /plans/provision/connect
adjust:
enabled: false
when: distro == fedora-rawhide

/disabled:
summary: Imported plan will be disabled by local plan
plan:
Expand Down
7 changes: 7 additions & 0 deletions tmt/base/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2626,6 +2626,13 @@ def plans(
plans = []
for plan in unresolved_plans:
try:
# Do not resolve disabled plans during `tmt run`
if run is not None and not plan.enabled:
Copy link
Copy Markdown
Member

@psss psss Apr 20, 2026

Choose a reason for hiding this comment

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

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.

plan.debug(
f"Plan {plan.name} is not enabled, skipping imports resolution",
Copy link
Copy Markdown
Member

@psss psss Apr 17, 2026

Choose a reason for hiding this comment

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

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.",

level=3,
)
continue
Copy link
Copy Markdown
Contributor

@happz happz Apr 14, 2026

Choose a reason for hiding this comment

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

This if block does not have to be under the try/except block.

Copy link
Copy Markdown
Member

@psss psss Apr 17, 2026

Choose a reason for hiding this comment

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

Agreed.

plans += plan.resolve_imports()
except Exception as error:
if self.import_before_name_filter:
Expand Down
Loading