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

Accept nested namespaces in spacewalk.api #57491

Merged
merged 3 commits into from
Sep 30, 2020

Conversation

agraul
Copy link
Contributor

@agraul agraul commented May 28, 2020

What does this PR do?

salt-run $server spacewalk.api allows users to run arbitrary Spacewalk API functions through Salt. These are passed in a namespace.method notation and may use nested namespaces. Previously only methods in a top-level namespace were supported.

What issues does this PR fix or reference?

Fixes: #57442

Previous Behavior

Exception occurred in runner spacewalk.api: Traceback (most recent call last):
  File "/usr/lib/python3.6/site-packages/salt/client/mixins.py", line 377, in low
    data['return'] = func(*args, **kwargs)
  File "/usr/lib/python3.6/site-packages/salt/runners/spacewalk.py", line 175, in api
    namespace, method = command.split('.')
ValueError: too many values to unpack (expected 2)

New Behavior

API gets called as expected, e.g.

channel.software.listChildren ('sle-product-sles15-sp1-pool-x86_64',):
    |_
      ----------
      arch_label:
          channel-x86_64
      arch_name:
          x86_64
[...]

Merge requirements satisfied?

Does this need to be documented in addition to the changelog? As there are no spacewalk runner tests yet I will need some time writing them, I opened the PR already to get feedback on the fix.

Commits signed with GPG?

Yes

@agraul agraul requested a review from a team as a code owner May 28, 2020 17:49
@ghost ghost requested review from garethgreenaway and removed request for a team May 28, 2020 17:49
@Akm0d Akm0d added the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label May 28, 2020
@Akm0d
Copy link
Contributor

Akm0d commented May 28, 2020

@agraul Thank you for opening a PR so quickly! Let me or @waynew know if you need help with writing the tests

salt/runners/spacewalk.py Outdated Show resolved Hide resolved
@agraul agraul force-pushed the spacewalk-runner-parse-command branch 2 times, most recently from 5b6a4e8 to f15a531 Compare May 29, 2020 09:10
@waynew
Copy link
Contributor

waynew commented May 29, 2020

An odd failure on centos happening here - I've asked the team if they've seen anything. I guess I can kick these off again and see if it was just an infrastructure thing

@agraul
Copy link
Contributor Author

agraul commented May 29, 2020

I'll push the tests on Tuesday (Monday is a public holiday where I live), I have something that works but I want to clean it up a little.

@agraul agraul force-pushed the spacewalk-runner-parse-command branch from f15a531 to 92f524d Compare June 2, 2020 14:13
@agraul
Copy link
Contributor Author

agraul commented Jun 2, 2020

Tests are there now, I got a pylint warning for the test:

tests/unit/runners/test_spacewalk.py:1: [W9903(no-encoding-in-file), ] PEP263: Use UTF-8 file encoding

With Python3 this can be left out in my opinion (with UTF-8 being the default), but I am not sure if this is an oversight w.r.t. the pylintrc or you want to keep it even for a Python3-only codebase. Let me know if I should add it.

@agraul agraul requested a review from waynew June 2, 2020 14:22
@waynew waynew removed the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Jun 15, 2020
Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

It's probably easiest for now to just go ahead and add the encoding line.

If we change it, it'll just be one more file for sed or awk to change. And that's the only thing that looks to need any changes here 👍

@agraul agraul force-pushed the spacewalk-runner-parse-command branch 2 times, most recently from 8b5d984 to fffa771 Compare June 16, 2020 08:13
@agraul agraul requested a review from waynew June 16, 2020 08:21
@agraul
Copy link
Contributor Author

agraul commented Jun 16, 2020

I've added the line, thanks for your review @waynew

waynew
waynew previously approved these changes Jun 16, 2020
Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

🚀

@sagetherage sagetherage added Bug broken, incorrect, or confusing behavior Magnesium Mg release after Na prior to Al Execution-Module severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around labels Jul 22, 2020
agraul and others added 3 commits September 29, 2020 16:06
salt-run $server spacewalk.api allows users to run arbitrary Spacewalk
API functions through Salt. These are passed in a namespace.method
notation and may use nested namespaces. Previously only methods in a
top-level namespace were supported.

Fixes saltstack#57442

Co-authored-by: Wayne Werner <wwerner@saltstack.com>
@agraul agraul force-pushed the spacewalk-runner-parse-command branch from 9fcb54c to 8d4ca7a Compare September 29, 2020 14:10
@dwoz dwoz merged commit 8a48b95 into saltstack:master Sep 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior Execution-Module has-failing-test Magnesium Mg release after Na prior to Al severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Spacewalk runner does not properly parse method namespaces
5 participants