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

Polish prepare/ansible documentation #2824

Merged
merged 1 commit into from
Apr 26, 2024
Merged

Conversation

happz
Copy link
Collaborator

@happz happz commented Apr 4, 2024

Includes added pyright cover, two birds with one stone.

Pull Request Checklist

  • implement the feature

@happz happz added documentation Improvements or additions to documentation code | no functional change "No Functional Change" intended. Patch should not change tmt's behavior in any way. test coverage Improvements or additions to test coverage of tmt itself labels Apr 4, 2024
@happz happz added this to the 1.33 milestone Apr 4, 2024
@martinhoyer
Copy link
Collaborator

looks good to me

@happz happz force-pushed the prepare-ansible-polish-docs branch from 5683d6c to 91793bc Compare April 10, 2024 11:26
@happz happz added the ci | full test Pull request is ready for the full test execution label Apr 10, 2024
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 nice, thanks! Just two nitpicks.

@psss psss requested a review from qcheng-redhat April 25, 2024 10:07
Includes added pyright cover, two birds with one stone.
@psss psss force-pushed the prepare-ansible-polish-docs branch from f9496fd to ff08000 Compare April 25, 2024 10:26
@qcheng-redhat
Copy link
Contributor

LGTM.

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.

The docs look good to me, just a semi-related question - Shouldn't tmt have ansible as a dependency in some form? afaik, it will only be installed as part of provision-virtual or provision-container optional extras packages. Then there is the PyPI package.
Maybe at least it should be mentioned in the docs with "make sure you have ansible installed" or something

@psss
Copy link
Collaborator

psss commented Apr 26, 2024

Shouldn't tmt have ansible as a dependency in some form? afaik, it will only be installed as part of provision-virtual or provision-container optional extras packages.

Good point! The cleanest way would probably be to require ansible from tmt+prepare-ansible subpackage as we don't want to install ansible with the minimal tmt package, right?

@happz
Copy link
Collaborator Author

happz commented Apr 26, 2024

Shouldn't tmt have ansible as a dependency in some form? afaik, it will only be installed as part of provision-virtual or provision-container optional extras packages.

Good point! The cleanest way would probably be to require ansible from tmt+prepare-ansible subpackage as we don't want to install ansible with the minimal tmt package, right?

And tmt+finish-ansible...

@psss
Copy link
Collaborator

psss commented Apr 26, 2024

Test failures are irrelevant --> merging.

@psss psss merged commit ff08000 into main Apr 26, 2024
14 of 20 checks passed
@psss psss deleted the prepare-ansible-polish-docs branch April 26, 2024 09:02
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 | no functional change "No Functional Change" intended. Patch should not change tmt's behavior in any way. documentation Improvements or additions to documentation test coverage Improvements or additions to test coverage of tmt itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants