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: Respect defaults when enabling multiple streams of a module #1152

Merged
merged 4 commits into from
Jan 29, 2024

Conversation

pkratoch
Copy link
Contributor

@pkratoch pkratoch commented Jan 9, 2024

Resolves: #1048

@pkratoch
Copy link
Contributor Author

pkratoch commented Jan 9, 2024

Related CI tests: rpm-software-management/ci-dnf-stack#1433

@j-mracek j-mracek self-assigned this Jan 17, 2024
if (!multiple_stream_modules.empty()) {
throw EnableMultipleStreamsError(
M_("Cannot enable multiple streams of one module at the same time. Affected modules: {}"),
utils::string::join(multiple_stream_modules, ", "));
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that in some aspects DNF4 provided a better message.

sudo dnf module enable '*' '*m*'
[sudo] password for jmracek: 
Last metadata expiration check: 2:04:24 ago on Wed 17 Jan 2024 12:17:10 PM CET.
Argument '*' matches 5 streams ('10.5', '10.6', '10.7', '10.8', '10.9') of module 'mariadb', but none of the streams are enabled or default
Unable to resolve argument *
Argument '*m*' matches 5 streams ('10.5', '10.6', '10.7', '10.8', '10.9') of module 'mariadb', but none of the streams are enabled or default
Unable to resolve argument *m*
Error: Problems in request:
broken groups or modules: *, *m*

Your code is better because it provides information about all modules with multiple stream for one spec.

sudo dnf5 module enable '*' '*m*'
Updating and loading repositories:
Repositories loaded.
Cannot enable multiple streams of one module at the same time. Affected modules: avocado, avocado-vt, cri-o, mariadb, nextcloud, nginx, perl, perl-DBD-SQLite, perl-libwww-perl, postgresql, swig

The new code does not show information about available streams available streams. It means that user has to run additional command to list all streams for spec.

The critical problem - DNF5 does not show error message for all arguments but only for the first one.
Important problem - if I am not mistaken (I am guessing, following error catch path is quite demanding) - problem is not logged into event log. It means API users will have a difficulty to understand the issue. I am investigation it and I will add additional information in future.

@pkratoch pkratoch force-pushed the enable-default branch 3 times, most recently from d60e8da to 1587ad9 Compare January 25, 2024 08:34
spec,
enable_ret.second,
log_level);
if (!skip_unavailable) {
Copy link
Contributor

@j-mracek j-mracek Jan 26, 2024

Choose a reason for hiding this comment

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

Are you sure that you want to use skip_unavailable option for situation when request is conflicting? What about skip_broken option.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have to test how code handles both situation - speck does not match any module and GoalProblem::MULTIPLE_STREAMS

Copy link
Contributor

Choose a reason for hiding this comment

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

After testing - scenario with no match works correctly. Only what works not nicely is a case where GoalProblem::MULTIPLE_STREAMS is triggered and option --skip-unavailable ignores the issue.

Copy link
Contributor

@j-mracek j-mracek Jan 26, 2024

Choose a reason for hiding this comment

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

This is a minor issue, not something significant, but if it will be not addressed in the PR, then an issue should be created.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the skip_broken option.

auto module_dict = create_module_dict(module_spec_to_query(base, module_spec));
// Keep only enabled or default streams if possible
auto multiple_stream_modules = prune_module_dict(module_dict);
// If there are any modules with multiple streams to be enabled, throw an error
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that the commend is not valid anymore. May I ask you to update it?

A new stream of the NoStaticContext module is added and it's also added
to defaults. The stream is needed so that there is a module with more
than one stream and one default stream. Because of changing the
defaults, many tests need to be updated, since the module stream gets
enabled whenever there is any enable operation.
@j-mracek
Copy link
Contributor

LGTM

@j-mracek j-mracek added this pull request to the merge queue Jan 29, 2024
Merged via the queue into rpm-software-management:main with commit 326e827 Jan 29, 2024
5 of 9 checks passed
@pkratoch pkratoch deleted the enable-default branch February 12, 2024 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Respect defaults when enabling multiple streams of a module
2 participants