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

Call bash only when necessary on macOS #54769

Merged
merged 22 commits into from
Dec 26, 2019
Merged

Conversation

cdalvaro
Copy link
Contributor

@cdalvaro cdalvaro commented Sep 26, 2019

What does this PR do?

This PR fixes a bug introduced on PR #54136 (and possibly on PR #54079 too) that calls /bin/bash when the user's default shell is not bash.

Also, users that have set a different version of bash as their default shell, they might be calling the wrong shell.

For example, for people with zsh as default shell the current behaviour is not right, neither for people with /usr/local/bin/bash as their main shell since the current code is invoking /bin/bash instead the right one.

New Behavior

The new behaviour introduced in this PR invokes the right bash shell only when the users default shell is bash.

This is done in this way to load the right environment as it was intended in PR #54136.

Tests written?

Commits signed with GPG?

@cdalvaro cdalvaro changed the title WIP: Call bash only when necessary on macOS Call bash only when necessary on macOS Sep 27, 2019
@cdalvaro cdalvaro force-pushed the 2019.2.1 branch 4 times, most recently from 185b7b8 to b6a77fc Compare October 5, 2019 16:57
@cdalvaro cdalvaro changed the base branch from 2019.2.1 to master October 5, 2019 17:16
@cdalvaro
Copy link
Contributor Author

cdalvaro commented Oct 5, 2019

Following @waynew advice from PR #54216

Hey @cdalvaro - as you may have heard, we're changing to a single branch release strategy. What does that mean for this PR?

Well, you've got tests so 👍

We will be working to migrate all PRs from non-master branches to master. If you'd like to get your PR in sooner, please rebase it against master, force push to your branch, and edit this PR to be against master. If you need any help with any of those steps, please let us know!

I have modified this PR too in order to fulfil the new release strategy.

@cdalvaro cdalvaro requested a review from a team as a code owner October 22, 2019 17:18
@ghost ghost requested a review from xeacott October 22, 2019 17:18
@cdalvaro
Copy link
Contributor Author

I would like to know if this PR will be finally merged

@cdalvaro
Copy link
Contributor Author

Please @xeacott, could you revise this PR, it is open for nearly two months.

Thank you in advance.

Copy link
Contributor

@weswhet weswhet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@cdalvaro
Copy link
Contributor Author

Please @xeacott, could you revise this PR, it solves an issue introduced in #54136.

By accepting this pull request, #54079 could be closed since it was duplicated in #54136 in order to rebased commits into 2019.2.1 instead of 2019.2

I mention @weswhet too, because I don't know if @xeacott is actively working on this project since I haven't heard from him for a long time.

Thank you all in advance!!

@dwoz dwoz merged commit 80c9650 into saltstack:master Dec 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants