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

Fix httpasswd result false positive in test mode #26026

Merged
merged 1 commit into from Aug 5, 2015
Merged

Fix httpasswd result false positive in test mode #26026

merged 1 commit into from Aug 5, 2015

Conversation

anlutro
Copy link
Contributor

@anlutro anlutro commented Aug 5, 2015

httpasswd.user_exists currently always shows up as having changed something if ran with test=True.

          ID: phpmyadmin-htpasswd
    Function: webutil.user_exists
        Name: admin
      Result: None
     Comment: User already known
     Started: 15:16:10.324431
    Duration: 4.742 ms
     Changes:

@jfindlay jfindlay added the Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged label Aug 5, 2015
@jfindlay
Copy link
Contributor

jfindlay commented Aug 5, 2015

@anlutro, is it possible to fix this in another way since the state system relies on the result being None in test mode?

@jfindlay
Copy link
Contributor

jfindlay commented Aug 5, 2015

#25721.

@anlutro
Copy link
Contributor Author

anlutro commented Aug 5, 2015

There might be an underlying issue I'm missing, but my impression is that "no change" should be represented as True regardless of test mode or not. Similar issue: #25978

@anlutro
Copy link
Contributor Author

anlutro commented Aug 5, 2015

The current behaviour of returning None was added here: b23c0fa#diff-acd69d8f060effeb82071b575ddee9bbR84

@jfindlay
Copy link
Contributor

jfindlay commented Aug 5, 2015

ret['result'] for state functions is a tristate, where True means 'the change was successful/changes made', False: 'not successful/no changes', and None: test mode. I do not know if this is formally documented somewhere yet, but there is this: #25143.

@jfindlay
Copy link
Contributor

jfindlay commented Aug 5, 2015

@anlutro, see #26032.

@anlutro
Copy link
Contributor Author

anlutro commented Aug 5, 2015

If that's correct then the other pull request I made with a similar change
should probably be reverted, and the highstate outputter changed, as true
is the only value that shows up as no change for me...

On Wed, 5 Aug 2015 17:37 Justin Findlay notifications@github.com wrote:

@anlutro https://github.com/anlutro, see #26032
#26032.


Reply to this email directly or view it on GitHub
#26026 (comment).

@jfindlay
Copy link
Contributor

jfindlay commented Aug 5, 2015

@anlutro, I think so, yes. I have expanded the results documentation again, correctly this time, I hope: #26042.

@anlutro
Copy link
Contributor Author

anlutro commented Aug 5, 2015

I'll take a closer look later tonight, hard to do on the phone!

On Wed, 5 Aug 2015 19:36 Justin Findlay notifications@github.com wrote:

@anlutro https://github.com/anlutro, I think so, yes. I have expanded
the results documentation again, correctly this time, I hope: #26042
#26042.


Reply to this email directly or view it on GitHub
#26026 (comment).

@jfindlay
Copy link
Contributor

jfindlay commented Aug 5, 2015

Sure, thank you for your work on these issues.

@anlutro
Copy link
Contributor Author

anlutro commented Aug 5, 2015

@jfindlay thanks for the clarification in the docs. I think my change is still correct.

If at any point in the function there are any changes, there will have been an early return. The changes I made are at a place in the code where no changes have been made, so True should be returned regardless of whether salt is in test mode or not.

It looks to me like the author of b23c0fa thought you were supposed to return None if there were no changes in test mode.

@jfindlay jfindlay added Minor Change Platform Relates to OS, containers, platform-based utilities like FS, system based apps State-Module and removed Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged labels Aug 5, 2015
@jfindlay
Copy link
Contributor

jfindlay commented Aug 5, 2015

@anlutro, you are correct. My understanding of ret['result'] was incorrect a few hours ago, but hopefully the documentation changes in #26042 make it clearer.

jfindlay added a commit that referenced this pull request Aug 5, 2015
Fix httpasswd result false positive in test mode
@jfindlay jfindlay merged commit fde2fb1 into saltstack:2015.8 Aug 5, 2015
@anlutro
Copy link
Contributor Author

anlutro commented Aug 5, 2015

Speaking as someone who wasn't clear on what it meant before, it definitely does. Cheers!

@anlutro anlutro deleted the fix-httpasswd_test_result branch March 23, 2016 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Platform Relates to OS, containers, platform-based utilities like FS, system based apps State-Module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants