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

[2018.3] Fix 2 bugs found in the file.check_perms function #48508

Merged
merged 2 commits into from Jul 11, 2018

Conversation

Projects
None yet
2 participants
@rallytime
Contributor

rallytime commented Jul 10, 2018

What does this PR do?

This PR fixes 2 bugs found in the file.check_perms function.

  • Removes the duplicate mode changes check. This was accidentally added during a merge forward from 2017.7 as PRs #48398 and #48399 made similar changes, and the duplicate mode section came in from the merge forward.
  • Move comment string join and test/changes check to bottom of file.check_perms

What issues does this PR fix or reference?

Fixes saltstack/salt-jenkins#1021

Previous Behavior

The integration.states.test_user.UserTest.test_user_if_present test was failing:

Traceback (most recent call last):
  File "/testing/tests/support/mixins.py", line 550, in assertSaltTrueReturn
    self.assertTrue(saltret)
  File "/usr/lib/python3.5/unittest/case.py", line 677, in assertTrue
    raise self.failureException(msg)
AssertionError: False is not true

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/testing/tests/integration/states/test_user.py", line 59, in test_user_if_present
    self.assertSaltTrueReturn(ret)
  File "/testing/tests/support/mixins.py", line 556, in assertSaltTrueReturn
    **(next(six.itervalues(ret)))
AssertionError: False is not True. Salt Comment:
An exception occurred in this state: Traceback (most recent call last):
  File "/testing/salt/state.py", line 1913, in call
    **cdata['kwargs'])
  File "/testing/salt/loader.py", line 1834, in wrapper
    return f(*args, **kwargs)
  File "/testing/salt/states/user.py", line 613, in present
    __salt__['file.mkdir'](val, pre['uid'], pre['gid'], 0o755)
  File "/testing/salt/modules/file.py", line 5554, in mkdir
    makedirs_perms(directory, user, group, mode)
  File "/testing/salt/modules/file.py", line 5659, in makedirs_perms
    int('{0}'.format(mode)) if mode else None)
  File "/testing/salt/modules/file.py", line 4590, in check_perms
    ret['comment'].append(
AttributeError: 'str' object has no attribute 'append'

New Behavior

The integration.states.test_user.UserTest.test_user_if_present test passes.

In 2018.3, file attribute checks were added to file.check_perms and then the mode checks were moved down lower in the function. However, there is some logic to handle original comments where the list of comments is joined into a single string. There is also some logic to return None when changes are
present.

This logic is (correctly) at the bottom of the file.check_perms function in the 2017.7 branch.

This commit moves that logic back down to the bottom of the file.check_perms function for the 2018.3 branch.

We need this logic at the bottom of the function for the following reasons:

  1. We must wait to detect if changes occur until the very end. If changes occur during the file attribute check, then the None return will not get set when it should be.
  2. We cannot join the comment strings together until all of the potential comments have been added to the comment list. Otherwise the function will stacktrace because you cannot append to a unicode type.

Tests written?

No - tests already written, which found this bug because it was failing.

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

rallytime added some commits Jul 10, 2018

Fix up bad merge - remove extra section of "mode" changes
This came through in a merge-forward due to #48398 and #48399

Both PRs moved the "mode" section in file.check_perms down lower,
but when the merge-forward went through, the section got added
back again.
Move comment string join and test/changes check to bottom of file.che…
…ck_perms

In 2018.3, file attribute checks were added to file.check_perms and then
the mode checks were moved down lower in the function. However, there is some
logic to handle original comments where the list of comments is joined into
a single string. There is also some logic to return `None` when changes are
present.

This logic is (correctly) at the bottom of the file.check_perms function in
the 2017.7 branch.

This commit moves that logic back down to the bottom of the file.check_perms
function for the 2018.3 branch.

We need this logic at the bottom of the function for the following reasons:

1. We must wait to detect if changes occur until the very end. If changes
   occur during the file attribute check, then the `None` return will not
   get set when it should be.
2. We cannot join the comment strings together until all of the potential
   comments have been added to the comment list. Otherwise the function
   will stacktrace because you cannot `append` to a unicode type.

@rallytime rallytime requested a review from garethgreenaway Jul 10, 2018

@rallytime rallytime merged commit 70e5fcb into saltstack:2018.3 Jul 11, 2018

8 of 11 checks passed

default Build finished.
Details
jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #6328 — FAILURE
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #24256 — FAILURE
Details
WIP ready for review
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #26540 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #18574 — SUCCESS
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #11298 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #23210 — SUCCESS
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #20381 — SUCCESS
Details
jenkins/pr/lint The lint job has passed
Details

@rallytime rallytime deleted the rallytime:fix-file-bug branch Jul 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment