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

modules: Add module info command #1061

Merged
merged 1 commit into from
Dec 7, 2023

Conversation

pkratoch
Copy link
Contributor

@pkratoch pkratoch commented Dec 4, 2023

No description provided.

@jan-kolarik
Copy link
Member

It would be great to enable (at least some of) the existing tests for module info and module list in our CI. Currently, the background for tests relies on the module install command, but this shouldn't be mandatory for all tests. I believe we can address this by preparing the phase with the dnf4 command. But I've also noticed the functionality for listing installed modules is missing, so perhaps we can just create a follow-up ticket for the tests to ensure it's not overlooked in the future?

Copy link
Member

@jan-kolarik jan-kolarik left a comment

Choose a reason for hiding this comment

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

LGTM

@j-mracek
Copy link
Contributor

j-mracek commented Dec 6, 2023

May I ask you to provide CI tests for the command? It is not a blocker for PR, but I recommend to have an external tracker.

@pkratoch
Copy link
Contributor Author

pkratoch commented Dec 6, 2023

Yes, I plan to enable the CI tests that use info and not install or other unimplemented commands (though these tests mostly test something else and just happen to use info) and I will create an issue for those that use the install command.

But I could also create at least one test specifically for info that doesn't use install until the install command is implemented, if you think it would be better...

@jan-kolarik
Copy link
Member

Yes, I plan to enable the CI tests that use info and not install or other unimplemented commands (though these tests mostly test something else and just happen to use info) and I will create an issue for those that use the install command.

But I could also create at least one test specifically for info that doesn't use install until the install command is implemented, if you think it would be better...

The issue is fine for now I'd say. And related to the missing install command, I believe we can resolve it by utilizing the Given I set dnf command to ... construct in the tests. This way, we can set up the preparation phase using dnf4 and then use dnf5 to test the info or list output. I've tested it briefly locally, and I can prepare a PR later against the issue you'll create.

@jan-kolarik jan-kolarik added this pull request to the merge queue Dec 7, 2023
Merged via the queue into rpm-software-management:main with commit 5a86637 Dec 7, 2023
11 of 14 checks passed
@pkratoch pkratoch deleted the info branch December 12, 2023 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants