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

[2015.8] [salt-ssh] Remove umask around actual execution for salt-ssh #29538

Merged
merged 1 commit into from Dec 8, 2015

Conversation

Projects
None yet
3 participants
@basepi
Collaborator

basepi commented Dec 8, 2015

Fixes #29486

Refs #29126

@fcrozat I'm reverting this piece of your umask fixes. It changes the umask for the whole command execution, which is too much for some setups. We could potentially make this configurable and add it back in (don't change the umask by default, that kind of thing).

basepi added a commit that referenced this pull request Dec 8, 2015

Merge pull request #29538 from basepi/salt-ssh.umask.29486
[2015.8] [salt-ssh] Remove umask around actual execution for salt-ssh

@basepi basepi merged commit 1d80144 into saltstack:2015.8 Dec 8, 2015

4 of 6 checks passed

default Merged build finished.
Details
jenkins/salt-pr-rs-cent6-n Salt PR - RS CentOS 6 #621 — FAILURE
Details
jenkins/salt-pr-clone Salt PR - Clone Repository #11877 — SUCCESS
Details
jenkins/salt-pr-linode-ubuntu14.04-n Salt PR - Linode Ubuntu 14.04 #2941 — SUCCESS
Details
jenkins/salt-pr-lint-n Salt PR - Code Lint #11582 — SUCCESS
Details
jenkins/salt-pr-rs-cent7-n Salt PR - RS CentOS 7 #10446 — SUCCESS
Details
@fcrozat

This comment has been minimized.

Show comment
Hide comment
@fcrozat

fcrozat Dec 9, 2015

Contributor

Yes, for my own usage, I'm able to change umask on the minion system and it is more strict than the default (and it is the exact opposite of the regression spotted in issue #29486)

Contributor

fcrozat commented Dec 9, 2015

Yes, for my own usage, I'm able to change umask on the minion system and it is more strict than the default (and it is the exact opposite of the regression spotted in issue #29486)

@basepi

This comment has been minimized.

Show comment
Hide comment
@basepi

basepi Dec 9, 2015

Collaborator

Yeah, I think our best bet will be to add some more formatting to the python thin to add a variable for setting the umask on the command. It should be fairly straightforward, can you create an issue and ping me on it @fcrozat?

Collaborator

basepi commented Dec 9, 2015

Yeah, I think our best bet will be to add some more formatting to the python thin to add a variable for setting the umask on the command. It should be fairly straightforward, can you create an issue and ping me on it @fcrozat?

@pykler

This comment has been minimized.

Show comment
Hide comment
@pykler

pykler Dec 16, 2015

Contributor

Moving forward, small suggestion. Any code that changes default behaviour, like setting something globally should not be allowed. One idea is anything like that should be introduced as a configuration option (or parameter to states) that changes the previous behaviour.

Contributor

pykler commented Dec 16, 2015

Moving forward, small suggestion. Any code that changes default behaviour, like setting something globally should not be allowed. One idea is anything like that should be introduced as a configuration option (or parameter to states) that changes the previous behaviour.

@basepi

This comment has been minimized.

Show comment
Hide comment
@basepi

basepi Dec 16, 2015

Collaborator

Sorry again for the inconvenience @pykler. We really need to add more tests around salt-ssh to try to catch things like this as well.

Collaborator

basepi commented Dec 16, 2015

Sorry again for the inconvenience @pykler. We really need to add more tests around salt-ssh to try to catch things like this as well.

@pykler

This comment has been minimized.

Show comment
Hide comment
@pykler

pykler Dec 16, 2015

Contributor

No apologies required, also I really appreciate the fast moving releases, except for when things break. I am not sure tests would fix this, the PR was approved and it essentially enforced behaviour that was not there before (in this case a non default umask). Fundamentally, this change should only have been approved if and only if it didnt create backward incompatible changes. One way to detect such changes, is the introduction of magic numbers / strings, in this case 0a077 ... those should be a red flag that require a deeper look, into what this new constant means with respect to old behaviour.

Contributor

pykler commented Dec 16, 2015

No apologies required, also I really appreciate the fast moving releases, except for when things break. I am not sure tests would fix this, the PR was approved and it essentially enforced behaviour that was not there before (in this case a non default umask). Fundamentally, this change should only have been approved if and only if it didnt create backward incompatible changes. One way to detect such changes, is the introduction of magic numbers / strings, in this case 0a077 ... those should be a red flag that require a deeper look, into what this new constant means with respect to old behaviour.

@basepi

This comment has been minimized.

Show comment
Hide comment
@basepi

basepi Dec 17, 2015

Collaborator

Yep, good points all. We'll try to be more careful going forward.

Collaborator

basepi commented Dec 17, 2015

Yep, good points all. We'll try to be more careful going forward.

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