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

Install dependencies of upgrade tasks #2866

Merged
merged 1 commit into from
May 6, 2024

Conversation

martinhoyer
Copy link
Collaborator

@martinhoyer martinhoyer commented Apr 16, 2024

Fixes #2643.

Pull Request Checklist

  • implement the feature
  • extend the test coverage
  • include a release note

@lukaszachy lukaszachy added this to the 1.33 milestone Apr 23, 2024
@psss
Copy link
Collaborator

psss commented Apr 30, 2024

@martinhoyer, could you please include a simple test checking that beakerlib would be installed even if the test which is executed is using shell as the framework? I'd suggest to create a slightly modified copy of /tests/execute/upgrade/simple which would select only the shell test:

rlRun -s "tmt -c upgrade-path="${PATH}" \
run --scratch -avvvdddi $run --rm --before finish \
plan -n /plan/no-path \
execute -h upgrade -t '/tasks/prepare' \
provision -h container -i fedora:${PREVIOUS_VERSION}" \
0 "Run a single upgrade task"

@lukaszachy
Copy link
Collaborator

Just a note that 'upgrade path' doesn't need to run distro update - it can be any plan. E.g. in #2643 (comment) it is quite fast.
We have 1 existing test which does the distro upgrade and that is IMO enough. All we need to do for this PR is to assert 'upgrade path' dependenicies are installed. No need to waste time in ugrading distro

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.

LGTM.

IMO also worth of release note

@martinhoyer
Copy link
Collaborator Author

Just a note that 'upgrade path' doesn't need to run distro update - it can be any plan. E.g. in #2643 (comment) it is quite fast. We have 1 existing test which does the distro upgrade and that is IMO enough. All we need to do for this PR is to assert 'upgrade path' dependenicies are installed. No need to waste time in ugrading distro

Thanks @lukaszachy , I'm trying to adapt your reproducer, but I'm not super sure how to modify it so it passes the tmt lint.

btw, I've found a small dummy-test-package-crested package to add as require in the upgrade task: Total download size: 11 k; Installed size: 6.9 k

@lukaszachy
Copy link
Collaborator

I'm trying to adapt your reproducer, but I'm not super sure how to modify it so it passes the tmt lint.

It smells with linter bug. I have same 'warn' reported for in plans for /tests/execute/upgrade (in /data).
Especially the warn C000 value of "how" is not "tmt" check...

@psss
Copy link
Collaborator

psss commented May 2, 2024

Just a note that 'upgrade path' doesn't need to run distro update - it can be any plan. E.g. in #2643 (comment) it is quite fast. We have 1 existing test which does the distro upgrade and that is IMO enough. All we need to do for this PR is to assert 'upgrade path' dependenicies are installed. No need to waste time in ugrading distro

Yes, and /tests/execute/upgrade/simple actually does not run the whole upgrade as it selects only /tasks/prepare from the upgrade repo which does almost nothing. My suggestion was to just do something like this:

tmt run --all -v
    plan --name /plan/no-path
    test --name /test/shell
    provision --how container --image fedora:39
    execute --how upgrade --test /tasks/prepare

Which should be fast enough and would verify that beakerlib is installed as the upgrade task requires it but /test/shell does not.

@martinhoyer martinhoyer force-pushed the upgrade_execute branch 2 times, most recently from ac3a670 to 0000771 Compare May 3, 2024 06:22
@martinhoyer martinhoyer marked this pull request as ready for review May 3, 2024 06:22
@psss psss added plugin | upgrade The upgrade execute plugin ci | full test Pull request is ready for the full test execution labels May 3, 2024
@psss
Copy link
Collaborator

psss commented May 3, 2024

/packit test

@psss
Copy link
Collaborator

psss commented May 3, 2024

IMO also worth of release note

Yeah, makes sense, added one in 376efe0 with some minor adjustments.

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.

Looks good, thanks for working on this. Added just a few minor suggestions.

tmt/steps/execute/upgrade.py Outdated Show resolved Hide resolved
tmt/steps/execute/upgrade.py Outdated Show resolved Hide resolved
tmt/steps/execute/upgrade.py Outdated Show resolved Hide resolved
@psss psss self-assigned this May 6, 2024
@psss psss merged commit 927da13 into teemtee:main May 6, 2024
19 of 20 checks passed
@mmacura311
Copy link

sanity tested this functionality with following copr builds:

tmt-1.33.dev29+g2879190a-main.fc40.noarch
tmt+export-polarion-1.33.dev29+g2879190a-main.fc40.noarch
tmt+report-junit-1.33.dev29+g2879190a-main.fc40.noarch
tmt+report-polarion-1.33.dev29+g2879190a-main.fc40.noarch
tmt+provision-beaker-1.33.dev29+g2879190a-main.fc40.noarch
tmt+provision-container-1.33.dev29+g2879190a-main.fc40.noarch
tmt+provision-virtual-1.33.dev29+g2879190a-main.fc40.noarch
tmt+test-convert-1.33.dev29+g2879190a-main.fc40.noarch
tmt+all-1.33.dev29+g2879190a-main.fc40.noarch

did run an upgrade test that require beakerlib in the pre-upgrade test already at the beginning, and second one where requirement is added only at the upgrade phase. Both tests passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci | full test Pull request is ready for the full test execution plugin | upgrade The upgrade execute plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The beakerlib package is missing for the upgrade step
4 participants