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

Allow kwargs for fileserver roots update (bsc#1218482) #618

Merged

Conversation

ycedres
Copy link
Contributor

@ycedres ycedres commented Jan 11, 2024

What does this PR do?

When an orchestration state is applied the user responsible for the runner is passed to the update function as the __pub_user keyword argument. Therefore, the update function must be aware of this or the orchestration state application will fail.

What issues does this PR fix or reference?

Fixes: https://github.com/SUSE/spacewalk/issues/23324
Upstream PR: saltstack/salt#65819

Previous Behavior

The application of this example orchestration state fails

my_orch:
  salt.runner:
    - name: fileserver.update

with the following error:

 Passed invalid arguments: update() got an unexpected keyword argument '__pub_user'

New Behavior

Now the orchestrating state is applied correctly.

Additionally, it fixes wrong asserts on fileserver.update tests, as it was producing false positives when ret["return"] is a string (i.a. when errors are produced) instead of True value.

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

Copy link
Member

@agraul agraul left a comment

Choose a reason for hiding this comment

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

Can you please add the link to the upstream PR to the description?

I left a few comments inline.

salt/fileserver/roots.py Outdated Show resolved Hide resolved
salt/fileserver/roots.py Outdated Show resolved Hide resolved
@ycedres ycedres force-pushed the fix/allow-kargs-fileserver-roots-update branch from 06cc4a4 to c218dda Compare January 12, 2024 10:12
@ycedres ycedres force-pushed the fix/allow-kargs-fileserver-roots-update branch from c218dda to fadbbf3 Compare January 12, 2024 11:13
@ycedres ycedres requested a review from agraul January 12, 2024 13:15
salt/fileserver/roots.py Outdated Show resolved Hide resolved
salt/fileserver/roots.py Outdated Show resolved Hide resolved
@ycedres ycedres requested a review from agraul January 17, 2024 17:47
Copy link
Contributor

@vzhestkov vzhestkov left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍
Thanks for taking care of the tests 👍

@meaksh meaksh merged commit 8ae54e8 into openSUSE/release/3006.0 Jan 23, 2024
3 checks passed
@meaksh meaksh deleted the fix/allow-kargs-fileserver-roots-update branch January 23, 2024 15:33
meaksh added a commit that referenced this pull request Oct 29, 2024
* Allow kwargs for fileserver roots update (bsc#1218482)

* Prevent exceptions with fileserver.update when called via state

* Fix wrong logic and enhance tests around fileserver.update

* Remove test which is not longer valid

---------

Co-authored-by: Pablo Suárez Hernández <[email protected]>
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.

4 participants