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

Add umask as a global state argument #57803

Merged
merged 2 commits into from
Nov 1, 2022
Merged

Conversation

terminalmage
Copy link
Contributor

This removes the umask argument from cmd states and makes it a global state argument.

@terminalmage terminalmage requested a review from a team as a code owner June 25, 2020 22:18
@ghost ghost requested review from garethgreenaway and removed request for a team June 25, 2020 22:18
@garethgreenaway
Copy link
Contributor

@terminalmage Looks good. My only comment is on the change in doc/ref/states/requisites.rst, why this is temporary? Also we need a changelog file added and the pre-commit test is failing.

@terminalmage
Copy link
Contributor Author

It's temporary because the umask is only changed for the lifetime of a single state.

Perhaps the wording can be improved?

@garethgreenaway
Copy link
Contributor

@terminalmage Thanks. That makes more sense. My first impression was that it was being changed for just the 3002 release and would change to a different approach in the future.

@terminalmage
Copy link
Contributor Author

terminalmage commented Jun 26, 2020

@garethgreenaway Cool. Should this just be considered an "added" changelog entry even though umask argument has been removed from the "cmd" state module here? The functionality will remain the same for cmd states.

@terminalmage
Copy link
Contributor Author

Also, can the changelog files contain markdown? This is not clear from the docs.

@garethgreenaway
Copy link
Contributor

@terminalmage I would make it an "changed" change since you're remove the umask argument from cmd in favor of it using the global one. It looks like town crier does have support for using markdown.

@terminalmage
Copy link
Contributor Author

OK, thanks. Changelog file added.

garethgreenaway
garethgreenaway previously approved these changes Jul 6, 2020
@Ch3LL
Copy link
Contributor

Ch3LL commented Oct 6, 2022

@terminalmage mind resolving the merge conflicts?

This removes the umask argument from cmd states and makes it a global
state argument.
@Ch3LL Ch3LL added Sulfur v3006.0 release code name and version and removed has-failing-test labels Nov 1, 2022
@terminalmage
Copy link
Contributor Author

Merge conflicts removed, tests re-done and passing

@Ch3LL Ch3LL merged commit 8915064 into saltstack:master Nov 1, 2022
@terminalmage terminalmage deleted the state-umask branch November 1, 2022 20:22
@max-arnold
Copy link
Contributor

The 3002 version number in the docs need to be changed to 3006: https://docs.saltproject.io/en/master/ref/states/requisites.html#run-state-with-a-different-umask

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Sulfur v3006.0 release code name and version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants