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

syspaths.py warning message #52352

Merged
merged 16 commits into from
Apr 26, 2019

Conversation

alan-cugler
Copy link
Contributor

What does this PR do?

  • sets missing variable to None
  • Created a warning level message:
    • lists missing variables
    • "typo?" prompt
    • shows how & where to explicitly set variables

What issues does this PR fix or reference?

#52230

Previous Behavior

  • If variable is missing from _syspaths.py file salt breaks and gives a traceback.

New Behavior

  • Default sets variables not explicitly set in the _syspaths.py file
  • Prompts user at log warning level about said variables.

Tests written?

  • Was manually tested, no automated tests written.

Commits signed with GPG?

Yes

… and default them to "None"

This will cause variables typoed or missing to be defaulted and allow salt to execute.
Added in typo question.
Added in example line to fix the issue.
@alan-cugler
Copy link
Contributor Author

I did not add in salt docs reference or URL since said doc doesn't exist yet, and reviewing other log messages showed no such references or URLs set in the messages.

@alan-cugler
Copy link
Contributor Author

I don't get the space after comma error in pylint. later in the same file the lists don't have spaces after commas? pylint test is just bullying me... (T_T)

@alan-cugler
Copy link
Contributor Author

alan-cugler commented Apr 4, 2019

@Ch3LL can you explain the error? I'm confused by it.

@Ch3LL
Copy link
Contributor

Ch3LL commented Apr 4, 2019

you will want to make sure you point your pylint to the testing pylintrc file. For example I run: python2 -m pylint --rcfile /home/ch3ll/git/salt/.testing.pylintrc tests/unit/utils/test_stringutils.py

This should give you similar results as these tests.

But to help point you in the right direction this is how you will need to fix this particular lint error:

diff --git a/salt/syspaths.py b/salt/syspaths.py
index 0e61da7639..46c104e9d3 100644
--- a/salt/syspaths.py
+++ b/salt/syspaths.py
@@ -27,7 +27,7 @@ svl = ('ROOT_DIR', 'CONFIG_DIR', 'CACHE_DIR', 'SOCK_DIR',  # salt variable list
     'SRV_ROOT_DIR', 'BASE_FILE_ROOTS_DIR', 'HOME_DIR',
     'BASE_PILLAR_ROOTS_DIR', 'BASE_THORIUM_ROOTS_DIR',
     'BASE_MASTER_ROOTS_DIR', 'LOGS_DIR', 'PIDFILE_DIR',
-    'SPM_PARENT_PATH', 'SPM_FORMULA_PATH','SPM_PILLAR_PATH',
+    'SPM_PARENT_PATH', 'SPM_FORMULA_PATH', 'SPM_PILLAR_PATH',
     'SPM_REACTOR_PATH', 'SHARE_DIR')
 
 try:

notice the space after the SPM_FORMULA_PATH comma

@alan-cugler alan-cugler closed this Apr 8, 2019
@alan-cugler alan-cugler reopened this Apr 8, 2019
@alan-cugler
Copy link
Contributor Author

Cool, not sure why it failed before, but now it passes all the tests!

salt/syspaths.py Outdated Show resolved Hide resolved
salt/syspaths.py Outdated Show resolved Hide resolved
salt/syspaths.py Show resolved Hide resolved
alan-cugler and others added 3 commits April 24, 2019 16:49
2) reversed single print for loop mechanic,
3) changed print structure based off of a suggestion by @waynew

Signed-off-by: Alan Cugler <alan.cugler101@gmail.com>
@alan-cugler
Copy link
Contributor Author

@waynew let me know if anything is still dis-satisfying for the PR. I ended up using %s instead of %r since the %r was printing the unicode with the key variable in the print.

@waynew
Copy link
Contributor

waynew commented Apr 25, 2019

@alan-cugler Looking great! Do you think it would make sense to add a test ensuring that the expected values do get set? I'm not sure if that's a test we could automate very well?

@alan-cugler
Copy link
Contributor Author

@waynew I am still learning your guy's test suite and how to use the mock library. We can open an issue to add a test in.

@waynew waynew merged commit 8bad5b1 into saltstack:develop Apr 26, 2019
garethgreenaway added a commit to garethgreenaway/salt that referenced this pull request Sep 19, 2019
@waynew waynew added the has master-port port to master has been created label Dec 2, 2019
dwoz added a commit that referenced this pull request Jan 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has master-port port to master has been created
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants