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

goal: Removal of not-installed group does not fail #1013

Merged
merged 2 commits into from
Nov 16, 2023
Merged

Conversation

m-blaha
Copy link
Member

@m-blaha m-blaha commented Nov 14, 2023

When the user attempts to remove a group that is not installed, only
print a warning but the resolvement should not fail.

Fixes: #917
Test: rpm-software-management/ci-dnf-stack#1406

if (action == GoalAction::REMOVE) {
// do not treat the removal of a not-installed group as an error
skip_unavailable = true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is minor. The call settings.resolve_skip_unavailable(cfg_main); will incorrectly set that for the request was used some setting, but if it is a remove action then it is ignored.

What about to do something like:
bool skip_unavailable = action == GoalAction::REMOVE ? true : settings.resolve_skip_unavailable(cfg_main);

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, fixed.

@j-mracek j-mracek self-assigned this Nov 14, 2023
@m-blaha m-blaha force-pushed the mblaha/group-remove branch from e1f756b to 28b61d7 Compare November 14, 2023 15:05
bool skip_unavailable = settings.resolve_skip_unavailable(cfg_main);
// For REMOVE action set skip_unavailable always to true
// to not treat the removal of a not-installed group as an error
bool skip_unavailable = action == GoalAction::REMOVE ? true : settings.resolve_skip_unavailable(cfg_main);
Copy link
Contributor

Choose a reason for hiding this comment

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

My apology, but I am thinking whether it is a good approach to hard code the behavior. skip_unavailable has 3 options - TRUE, FALSE, AUTO.

What about to add additional condition to action == GoalAction::REMOVE and check whether settings.skip_unavailable == GoalSetting::AUTO? And only if both conditions are
true then use skip_unavailable=true, otherwise use settings.resolve_skip_unavailable(cfg_main)

This change will allow API users to set behavior according to their expectations, but will keep reasonable default.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, that make sense. I'll change also package removal accordingly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

When the user attempts to remove a group that is not installed, only
print a warning but the resolvement should not fail.

Fixes: #917
@m-blaha m-blaha force-pushed the mblaha/group-remove branch from 28b61d7 to e191760 Compare November 15, 2023 09:16
Copy link
Contributor

@j-mracek j-mracek left a comment

Choose a reason for hiding this comment

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

LGTM, but I have to test it.

@m-blaha m-blaha force-pushed the mblaha/group-remove branch from e191760 to 8cbd697 Compare November 15, 2023 09:21
libdnf5/base/goal.cpp Outdated Show resolved Hide resolved
This change enables API users to set stricter behavior for
goal::add_remove() method regarding removal of not-installed packages.
If skip_unavailable is not explicitly set, the default behavior remains
to not error out.
@m-blaha m-blaha force-pushed the mblaha/group-remove branch from 8cbd697 to 1407909 Compare November 15, 2023 09:30
@j-mracek
Copy link
Contributor

Tested locally and it worked.

@j-mracek j-mracek added this pull request to the merge queue Nov 16, 2023
Merged via the queue into main with commit 3701d5d Nov 16, 2023
12 of 15 checks passed
@j-mracek j-mracek deleted the mblaha/group-remove branch November 16, 2023 13:44
mkrizek added a commit to mkrizek/ansible that referenced this pull request Nov 20, 2023
bcoca pushed a commit to ansible/ansible that referenced this pull request Nov 21, 2023
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.

Removing a group that is not installed considered a failure (rc == 1), dnf4 vs dnf5 inconsistency
2 participants