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

Support --workdir-root in the tmt clean command #2850

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

skycastlelily
Copy link
Collaborator

@skycastlelily skycastlelily commented Apr 12, 2024

Pull Request Checklist

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

tmt/base.py Outdated Show resolved Hide resolved
@skycastlelily
Copy link
Collaborator Author

Hi Petr, @psss, would you please spare some time to review this merge request,please? This is supposed to address #2807 , sorry for forgetting mentioned it before 1.36 release,and hope we can have this merged for 1.37: )

@skycastlelily skycastlelily added this to the 1.37 milestone Sep 25, 2024
@skycastlelily skycastlelily added the ci | full test Pull request is ready for the full test execution label Sep 25, 2024
@thrix thrix requested a review from happz September 26, 2024 11:36
docs/releases.rst Outdated Show resolved Hide resolved
docs/releases.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

@martinhoyer martinhoyer left a comment

Choose a reason for hiding this comment

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

lgtm. Just nitpicks.
I'm surprised setting a workdir root is so complicated tbh (not necessarily related to these changes). :)

tmt/cli.py Outdated Show resolved Hide resolved
tmt/utils/__init__.py Outdated Show resolved Hide resolved
@thrix
Copy link
Collaborator

thrix commented Sep 26, 2024

@skycastlelily would it be possible to log the workdir root please in case the verbose mode is enabled please?

Something like:

❯ env | grep WORKDIR
TMT_WORKDIR_ROOT=/var/home/mvadkert/.local/share/tmt

❯ tmt clean -v
clean
    workdir /var/home/mvadkert/.local/share/tmt
guests
runs
images
    testcloud

tmt/base.py Outdated Show resolved Hide resolved
tmt/base.py Outdated Show resolved Hide resolved
tmt/utils/__init__.py Outdated Show resolved Hide resolved
tmt/options.py Outdated Show resolved Hide resolved
@psss
Copy link
Collaborator

psss commented Nov 21, 2024

I've also moved the release note to the proper section and adjusted the wording a bit.

Seems the release note adjustment has been lost during rebase, added back in 7a0adc4. Please fetch the latest content from the branch before force-pushing new changes.

There is a couple of tests failing:

/tests/clean/guests
/tests/clean/runs
/tests/status/base

The error seems to be related:

status() missing 1 required positional argument: 'workdir_root'

Please, have a look. Thanks. Not ready for 1.39, moving to 1.40.

@psss psss modified the milestones: 1.39, 1.40 Nov 21, 2024
@skycastlelily
Copy link
Collaborator Author

skycastlelily commented Nov 22, 2024

Please fetch the latest content from the branch before force-pushing new changes

Will keep that on mind:)

The error seems to be related:

I guess it is because my commit to address that error is somehow removed during the rebase,anyway,gonna to have it back:)

@skycastlelily
Copy link
Collaborator Author

I guess it is because my commit to address that error is somehow removed

Yeah, that's the problem, added it back:4b61e18.

Not ready for 1.39, moving to 1.40.

We have lots of reviews, and commits for this mr, the "new" error is due to the commit to address the error is gone during rebase,hmm,lead, would you please,or do you see any chance we could have this mr released with 1.39 to avoid rebase mess 😿 🥺 ? 😁

@psss
Copy link
Collaborator

psss commented Nov 22, 2024

Sorry for that. I was ready to include it in the release but tests were red. And we cannot postpone the release indefinitely. Let's merge the pull request as one of the first ones in 1.40 to prevent any further rebase problems.

@psss psss added status | ready for merge The only missing piece is to do the rebase the current 'main' and let the CI finish. and removed status | need tests Test coverage to be added for the affected code labels Nov 22, 2024
'--finished', is_flag=True, default=False,
help='List all runs which have all enabled steps completed.')
@verbosity_options
def status(
Copy link
Collaborator

Choose a reason for hiding this comment

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

The status definition has been moved to a separate file status.py. Please, move the changes there.

@psss psss removed the status | ready for merge The only missing piece is to do the rebase the current 'main' and let the CI finish. label Nov 22, 2024
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 code | cli Changes related to the command line interface command | clean tmt clean command
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants