[master] Update pkg.group_installed state to support repo options #64349
Conversation
salt-project-bot-prod-environment
bot
changed the title
|
This is currently marked WIP, because it will require changes in salt/modules/pacmanpkg.py, and further tests need to be added. |
terminalmage
changed the title
|
This is no longer WIP and is ready for review. |
| @@ -2581,6 +2581,10 @@ def group_info(name, expand=False, ignore_groups=None): | |||
| to ``mandatory``, ``optional``, and ``default`` for accuracy, as | |||
| environment groups include other groups, and not packages. Finally, | |||
| this function now properly identifies conditional packages. | |||
| .. versionchanged:: 3006.2 | |||
There was a problem hiding this comment.
This seems like its actually a feature, which resolves a bug so this would need to be 3007.0, same for the 3006.2 references below
There was a problem hiding this comment.
The documentation for the pkgrepo.absent state says that you can pass anything you would pass through to the execution module, which is not true. IMO, that's a bug. Promised functionality doesn't exist. This modification is in service of fixing that bug.
There was a problem hiding this comment.
Sorry, had my other PR in mind, I meant pkg.group_installed, not pkgrepo.absent.
Here's the relevant block of code: https://github.com/saltstack/salt/blob/c33b562/salt/states/pkg.py#L3402-L3406
| pkg.group_installed: | ||
| - fromrepo: base,updates | ||
|
|
||
| .. versionadded:: 3006.2 |
There was a problem hiding this comment.
So, if on https://github.com/saltstack/salt/pull/64349/files#r1210742029 we can reasonable say it's a bug, because of your argument, why is it versionadded here?
There was a problem hiding this comment.
Because support for these arguments needed to be added to make the code agree with the documentation: https://github.com/saltstack/salt/blob/c33b562/salt/states/pkg.py#L3402-L3406
Is it no longer considered a bug when the code doesn't do what the documentation says? I'm confused.
There was a problem hiding this comment.
Is it no longer considered a bug when the code doesn't do what the documentation says? I'm confused.
It is, of course.
But can I argue that the function changed to accommodate the missing(and promised support) for the extra arguments?
There was a problem hiding this comment.
But can I argue that the function changed to accommodate the missing(and promised support) for the extra arguments?
I don't think that has ever been in dispute.
What seems to be in dispute though (unless I misunderstood your question), is whether adding arguments to a function to fix a bug makes the PR no longer a bugfix.
There was a problem hiding this comment.
I was suggesting from versionadded to versionchanged not that this is not code added to fix a bug.
Ignore my rant, I agree with the versionadded usage here.
There was a problem hiding this comment.
In that case, yes, I definitely misunderstood your earlier comment (sorry about that).
My thinking behind using versionadded is that prior releases did not support those arguments. I can see how the documentation could be confusing though. At the top of the docstring there is a versionchanged which describes the repo options, but the options themselves have versionadded markers.
I'm happy with whichever solution makes the docs make the most sense.
|
@terminalmage you okay if we close this now that #64994 has been merged in? It will get merged forward to master. |
|
Sure, thanks! |
What does this PR do?
What issues does this PR fix or reference?
Fixes #64348
Previous Behavior
Adding repo options causes state to fail
New Behavior
Adding repo options no longer causes state to fail
Merge requirements satisfied?
[NOTICE] Bug fixes or features added to Salt require tests.
Commits signed with GPG?
Yes/No
Please review Salt's Contributing Guide for best practices.
See GitHub's page on GPG signing for more information about signing commits with GPG.