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

Add 'noaction' flag for install/remove in the opkg execution module #50306

Merged
merged 10 commits into from Oct 31, 2018
Merged

Add 'noaction' flag for install/remove in the opkg execution module #50306

merged 10 commits into from Oct 31, 2018

Conversation

rares-pop
Copy link
Contributor

@rares-pop rares-pop commented Oct 30, 2018

What does this PR do?

Adding the 'noaction' flags when installing/removing software. Basically it's just testing if the operations will succeed or not without actually doing the operation.

Previous Behavior

opkg wasn't providing this functionality.

New Behavior

you can pass the noaction=True flag

Tests written?

Yes

Commits signed with GPG?

Yes

Rares POP added 2 commits Oct 29, 2018
@ghost ghost self-requested a review Oct 30, 2018
@rares-pop rares-pop changed the title Dev/iepopr/opkg noaction flag Add 'noaction' flag to the opkg execution module Oct 30, 2018
@rares-pop rares-pop changed the title Add 'noaction' flag to the opkg execution module Add 'noaction' flag for install/remove in the opkg execution module Oct 30, 2018
Copy link
Contributor

@cachedout cachedout left a comment

LGTM

Copy link
Contributor

@isbm isbm left a comment

But why? Isn't that we have test=True for this instead?
@cachedout while code looks OK, it is conceptually weird. This should be incorporated with the test=True instead.

@rares-pop
Copy link
Contributor Author

@rares-pop rares-pop commented Oct 30, 2018

@isbm - I am not aware of test=True. What is it?

I browsed dpkg.py and apk.py and I haven't seen anything interesting.

Are you saying that we shouldn't go with 'noaction' since that's very opkg specific and say 'test' instead?

@isbm
Copy link
Contributor

@isbm isbm commented Oct 31, 2018

@rares-pop this is a test mode, like dry run. This is not really working if module does not respects that (and you also don't know it does respect it or not...). Still this is how Salt is dealing here as of today. Basically, you're looking for __opts__['test'], and if it is there and True, then you no-op. So I would rather drop your own parameter and simply incorporate that path instead. Example you can find here, here, here or here. Or here.

@rares-pop
Copy link
Contributor Author

@rares-pop rares-pop commented Oct 31, 2018

@isbm - opts is the minion configuration, right? Can you specify that option (opts['test']) on a salt-call basis?

i.e. the sequence:
salt-call --local pkg.install 'foo'
salt-call --local pkg.install 'bar' test=True
salt-call --local pkg.install 'baz'

@isbm
Copy link
Contributor

@isbm isbm commented Oct 31, 2018

@isbm - optsis the minion configuration, right? Can you specify that option (opts['test']) on a salt-call basis?

i.e. the sequence:
salt-call --local pkg.install 'foo'
salt-call --local pkg.install 'bar' test=True
salt-call --local pkg.install 'baz'

Generally, you should be able to incorporate this with the testing mode, described here:
https://docs.saltstack.com/en/latest/ref/states/testing.html

@rares-pop
Copy link
Contributor Author

@rares-pop rares-pop commented Oct 31, 2018

@isbm - I don't want to go through state modules. I need to do a simple salt-call --local and pass that argument.

salt-call doesn't seem to put its arguments inside opts, but only in kwargs.

Can you be more specific on how I can achieve that with what you suggest (directly on the minion, possibly a masterless minion)?

@isbm
Copy link
Contributor

@isbm isbm commented Oct 31, 2018

First of all, you have to. 😉 It is a virtual module that is also used in states. So if state.apply gets it in test mode, your stuff must cope with it.

Second, if you want that also in the module call it do nothing, then just add **kwargs to the signature (or test=False directly) and use it accordingly, so the test=true mode is still there. Problem is that there is no much of standard so far and we're working to sort this out. However, let's not do another ideological forks. 😉

@rares-pop
Copy link
Contributor Author

@rares-pop rares-pop commented Oct 31, 2018

@isbm - sorry for the back and forth, but just to be sure to implement the correct thing:

  • salt/modules/opkg.py already has the kwargs in its signatures
  • given the current PR, you want me to rename the 'noaction' thing to 'test' and in addition, also check the 'opkg['test']

Is that correct?

If that's correct, if things contradict (opts['test'] = False but kwargs contain test=True'), which should have priority? Probably opts if it's set to true, otherwise kwargs if present?

@isbm
Copy link
Contributor

@isbm isbm commented Oct 31, 2018

@isbm - sorry for the back and forth, but just to be sure to implement the correct thing:

  • salt/modules/opkg.py already has the kwargs in its signatures

Then just pull "test" out of them:

 if bool(kwargs.get('test') or __opt__.get('test')):
        cmd_prefix.append('--noaction')

Rares POP added 4 commits Oct 31, 2018
Be compliant with Salt's terminology

Signed-off-by: Rares POP <rares.pop@ni.com>
Signed-off-by: Rares POP <rares.pop@ni.com>
tests/unit/modules/test_opkg.py Outdated Show resolved Hide resolved
tests/unit/modules/test_opkg.py Outdated Show resolved Hide resolved
Keep 'noaction' in the test name
since test_install_test sounds wrong.

Signed-off-by: Rares POP <rares.pop@ni.com>
@rares-pop
Copy link
Contributor Author

@rares-pop rares-pop commented Oct 31, 2018

Oops.. I kept the 'noaction' in the test name, because that's really what it should happen, even if the actual parameter is 'test=True'. More than that, test_install_test sounds weird.

Signed-off-by: Rares POP <rares.pop@ni.com>
isbm
isbm approved these changes Oct 31, 2018
@@ -269,6 +269,14 @@ def refresh_db(failhard=False, **kwargs): # pylint: disable=unused-argument
return ret


def _append_noaction_if_testmode(cmd, **kwargs):
Copy link
Contributor

@isbm isbm Oct 31, 2018

Choose a reason for hiding this comment

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

You can call it just _noaction 😉 but this is also OK.

@cachedout cachedout merged commit f58efef into saltstack:develop Oct 31, 2018
10 of 11 checks passed
garethgreenaway added a commit to garethgreenaway/salt that referenced this issue Sep 18, 2019
@waynew waynew added this to PR needs port to master in PRs to port to master Oct 24, 2019
dwoz added a commit that referenced this issue Nov 12, 2019
@garethgreenaway garethgreenaway moved this from PR needs port to master to PR merged in PRs to port to master Mar 24, 2020
@garethgreenaway garethgreenaway moved this from PR merged to PR has port to master in PRs to port to master Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
PRs to port to master
  
PR has port to master
Development

Successfully merging this pull request may close these issues.

None yet

4 participants