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

Seperate prlctl and prlsrvctl checks into each requiring function #49720

Merged
merged 5 commits into from Sep 21, 2018

Conversation

Projects
None yet
3 participants
@cstarke
Copy link

commented Sep 20, 2018

What does this PR do?

Split and move the availability checks for prlctl or prlsrvctl to each function requiring it.

What issues does this PR fix or reference?

#49709

Previous Behavior

Loading the Parallels module will fail if the prlsrvctl binary can't be found, whereas prlsrvctl is only required for one function: prlsrvclt().
All other functions can't be used.

New Behavior

Only prlsrvctl() requires the prlsrvctl binary.
prlctl() (invoked by all other functions) require the prlctl binary.
They can fail with a CommandExecutionError.

Tests written?

No

Commits signed with GPG?

No

@rallytime
Copy link
Contributor

left a comment

This looks fine to me, but I think this would be a good opportunity to document the requirements more clearly as dependencies.

I see at the top of the file there are statements about what is or is not built with the various prlctl/prlsrvctl commands, but we don't say that either library is a dependency.

Can you add that to the docs?

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2018

@cstarke Welcome! I have a small request above.

It also looks like this change is causing some tests to fail. Can you take a look?

https://jenkinsci.saltstack.com/job/pr-kitchen-ubuntu1604-py2/job/PR-49720/2/

@cstarke

This comment has been minimized.

Copy link
Author

commented Sep 20, 2018

Mh, I'm not sure I understand correctly. Do you have an example of this kind of documentation?

And I'll look into the failing tests :) Thanks

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2018

@cstarke Sorry, I was not being clear.

I just mean that we should document somewhere that some functions require one library or the other.

That way people don't have to run the function and hit the CommandExecutionError. It should be stated clearly at the top. Something like:

This module requires the prlctl binary to be installed to run most functions.

To run parallels.prlsrvctl, the prlsrvctl binary is required.

Or something along those lines.

@cstarke

This comment has been minimized.

Copy link
Author

commented Sep 20, 2018

@rallytime Ah okey! But you would just use the comment? Not re-implement the checks within virtual and only check for prlctl right?

cstarke
@rallytime

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2018

@cstarke I am just looking for a documentation clarification in the introduction at the top.

@rallytime
Copy link
Contributor

left a comment

Thank you @cstarke!

@rallytime rallytime merged commit e7bbb83 into saltstack:2018.3 Sep 21, 2018

7 of 10 checks passed

jenkins/pr/py2-centos-7 running py2-centos-7...
Details
jenkins/pr/py2-windows-2016 running py2-windows-2016...
Details
jenkins/pr/py3-windows-2016 running py3-windows-2016...
Details
WIP ready for review
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has passed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.