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 issues with functions that use makedirs on Windows #47533

Merged
merged 2 commits into from Jun 6, 2018

Conversation

Projects
None yet
5 participants
@twangboy
Contributor

twangboy commented May 8, 2018

What does this PR do?

Fixes issues with the file state module when makedirs is True on Windows

What issues does this PR fix or reference?

#47178

Previous Behavior

Functions would stack trace on Windows due to differences in the parameters expected by the file.makedirs and file.makedirs_perms functions on Windows and Linux.

New Behavior

It passes... on Windows...

Tests written?

No

Commits signed with GPG?

Yes

clean,
require,
exclude_pat,
user=None,

This comment has been minimized.

@cachedout

cachedout May 8, 2018

Contributor

This scares me a little bit. Why is it necessary to set all the defaults suddenly?

This comment has been minimized.

@twangboy

twangboy May 8, 2018

Contributor

There are two instances where this function is called. It only specifies the name of the directory. All other values are hard values (no variables). Those are here:

user=None,
group=None,
dir_mode=None,
win_owner=None,

This comment has been minimized.

@cachedout

cachedout May 8, 2018

Contributor

I don't understand here what distinguishes win_owner from user. Why do these need to be distinct?

This comment has been minimized.

@twangboy

twangboy May 8, 2018

Contributor

Maybe if you have a state that is managing a file on both Windows and Linux... Honestly I don't know why. I'm happy to use user. If win_owner isn't passed, it will use user. But we've been using win_owner for a while. Just keeping with the paradigm I guess.

This comment has been minimized.

@twangboy

twangboy May 8, 2018

Contributor

I believe it was a design decision Dave and I came up with... but I don't remember the reasons.

This comment has been minimized.

@cachedout

cachedout May 17, 2018

Contributor

@UtahDave Do you know why?

This comment has been minimized.

@UtahDave

UtahDave May 17, 2018

Member

I just went back and looked through the commit history a little bit. If win_owner isn't specified, then user is used.

My memory is a little hazy on the specifics since this was committed two years ago, but I believe it's because it was getting too difficult to manage Windows file system permissions by shoehorning it into Salt's Linux file system permission management functions.

This comment has been minimized.

@cachedout

cachedout May 29, 2018

Contributor

K. If it's a standard at this point then it is what it is. Thanks for the explanation.

@rallytime

This comment has been minimized.

Contributor

rallytime commented May 8, 2018

@twangboy does this really need to go in for 2017.7.6 and block that release? Or is this a nice-to-have and can go in for 2017.7.7?

@twangboy twangboy changed the title from Fix issues with functions that user makedirs on Windows to Fix issues with functions that use makedirs on Windows May 8, 2018

@twangboy twangboy changed the base branch from 2017.7.6 to 2017.7 May 8, 2018

@rallytime rallytime requested a review from cachedout May 15, 2018

@cachedout

I think I'm all right with this now but would like another core reviewer to have a look to double-check my work.

@rallytime rallytime requested a review from terminalmage May 29, 2018

@rallytime rallytime merged commit d56ddad into saltstack:2017.7 Jun 6, 2018

6 of 9 checks passed

default Build finished.
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #9742 — FAILURE
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #18832 — FAILURE
Details
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #24952 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #17070 — SUCCESS
Details
jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #4777 — SUCCESS
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #22702 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #21690 — SUCCESS
Details

@twangboy twangboy deleted the twangboy:fix_47178 branch Jun 6, 2018

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