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

Support additional eselect parameters. #33876

Merged
merged 2 commits into from
Jun 9, 2016
Merged

Conversation

jwagoner0
Copy link
Contributor

@jwagoner0 jwagoner0 commented Jun 8, 2016

What does this PR do?

Allows action_parameter to be passed in to get_target_list in the eselect module.
Passes the action_parameter from the set command in the eselect state to get_target_list.

What issues does this PR fix or reference?

The PHP eselect module requires an additional parameter to be passed in to list.
e.g. eselect php list cli

Other eselect modules that require an action_parameter, like java-vm, don't require any additional parameters for list. However, they seem to ignore it, so this change doesn't hurt any modules I tested.

Previous Behavior

The following config would always fail because the old code couldn't get a list of targets for PHP.

php:
  eselect.set:
    - target: php7.0
    - action_parameter: cli

New Behavior

The above config now properly sets the PHP version.

Tests written?

No

requires the type to be passed in.  Other eselect modules appear to
ignore any params passed to list.
e.g. eselect php list cli

Change set command in state to pass action_parameter to get_target_list.
@jwagoner0
Copy link
Contributor Author

The Jenkins failures don't look related to this change.

'''
List available targets for the given module.

module
name of the module to be queried for its targets

action_parameter
additional params passed to the defined action

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a .. versionadded:: Carbon tag here for this new kwarg?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I added a new commit.

@rallytime
Copy link
Contributor

Hi @jwagoner0 - you're right. Those test failures are not related to your change. I had one small comment that should be addressed and then we can get this in. Otherwise this looks great!

@rallytime rallytime merged commit 16e61d5 into saltstack:develop Jun 9, 2016
gitebra pushed a commit to gitebra/salt that referenced this pull request Jun 9, 2016
* upstream/develop: (211 commits)
  Fix merge error caught by tests
  Add boto3 to utils call
  Pylint fix
  Ensure tht pillar have freshest grains (saltstack#33910)
  Support additional eselect parameters. (saltstack#33876)
  Ensure tht pillar have freshest grains
  Add note about Xenial packages to 2016.3.0 release notes (saltstack#33870)
  add 2016.3.1 release notes (saltstack#33883)
  Fixup new groupadd tests for syntax change in 2016.3
  Use errno
  Set master and cloud to log level warning (saltstack#33861)
  Correct silly mistake
  Allow socket closes when the socket is disconnected
  Add support for edge case when Cmd and Entrypoint can't be blanked
  Remove matcher tests
  Fix another unit test stacktrace in pkg_resource
  Fixing more stupid unit tests
  Fix some more lint
  Fix/add unit tests for state
  Fix broken locate.locate function
  ...
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.

None yet

2 participants