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

--state-output and overriding defaults #25785

Merged
merged 2 commits into from
Aug 3, 2015
Merged

--state-output and overriding defaults #25785

merged 2 commits into from
Aug 3, 2015

Conversation

htch
Copy link
Contributor

@htch htch commented Jul 28, 2015

It seems like there's an issue with the way salt treats command-line arguments in general, i only run into this with --salt-output however. I believe it's not the intended behaviour – that would be odd if true.

Specific issue

I set "state_output: mixed" in /etc/salt/master so i expect state_output to be "mixed" unless i pass --state-output with a different value. It works as expected if i pass "--state-output terse" like this:

salt '*' state.sls somestate --state-output terse -l debug

However it does not work as expected if i pass "--state-output full":

salt '*' state.sls somestate --state-output full -l debug

General issue

As far as i understand the code, salt loads default options from several configuration files, reads command line arguments and then merges the options giving preference to command-line arguments; it looks reasonable so far. The problem is that as it turns out it is not always the case. A special clause here prefers the option from the configuration file if the option from the command line wasn't set (supposedly), this is necessary to prevent command-line defaults from overriding options explicitly set in the configuration file. What makes this code broken is that it does not really check if the command-line argument was really passed, only that its value should be equal to the command-line default as if it was the same thing.

@Cptn727
Copy link

Cptn727 commented Jul 28, 2015

I was able to reproduce this as described. I think @htch meant for the second listed command to different from the first by requesting the default (full) state output. I did the following:

salt -lquiet --state_output=terse '*' state.sls salt-minion.tasks test=false

Command output was terse.

I then followed this with:

salt -lquiet --state_output=full '*' state.sls salt-minion.tasks test=false

Command output appeared to be 'changes' as set in /etc/salt/master.d/output.conf (thus ignoring the value specified in the command).

Version report information:

                  Salt: 2015.5.2
                Python: 2.7.6 (default, Jun 22 2015, 17:58:13)
                Jinja2: 2.7.2
              M2Crypto: 0.21.1
        msgpack-python: 0.3.0
          msgpack-pure: Not Installed
              pycrypto: 2.6.1
               libnacl: Not Installed
                PyYAML: 3.10
                 ioflo: Not Installed
                 PyZMQ: 14.0.1
                  RAET: Not Installed
                   ZMQ: 4.0.4
                  Mako: 0.9.1
 Debian source package: 2015.5.2+ds-1trusty1

@htch
Copy link
Contributor Author

htch commented Jul 28, 2015

Attaching pull request that attempts to fix the issue. What it does is overriding default optparse.Option so that it can be checked if command-line option was really passed by the user rather than if its value equals optparse' default.

@jfindlay jfindlay added Core relates to code central or existential to Salt Medium Change Tests-Passed labels Jul 28, 2015
@cachedout
Copy link
Contributor

@s0undt3ch Could you take a look here, please?

@s0undt3ch
Copy link
Collaborator

Go Go Jenkins!

@s0undt3ch
Copy link
Collaborator

I don't have any issues with the fix implemented here, the alternative would probably involve a bit more of logic, so, once tests pass, this is good to go in.

s0undt3ch added a commit that referenced this pull request Aug 3, 2015
--state-output and overriding defaults
@s0undt3ch s0undt3ch merged commit 4b3b442 into saltstack:develop Aug 3, 2015
DmitryKuzmenko pushed a commit to DSRCorporation/salt that referenced this pull request Sep 30, 2015
Old (PR saltstack#25785): set option_class static value on class instantiation level. This
was overwritten by OptionParser.__init__ on instantiation. So it worked
for option groups but not worked for root level options.

New: pass option_class as kwarg to OptionParser.__init__.
@DmitryKuzmenko
Copy link
Contributor

@s0undt3ch I've fixed an issue in this solution, there are just a couple of strings, could you please take a look at #27558?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core relates to code central or existential to Salt Tests-Passed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants