Skip to content

Update scripts to be compatible with sh #22

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

Merged
merged 1 commit into from
Aug 29, 2023

Conversation

peterwald
Copy link
Contributor

The usage of >& redirection operators in the install and entrypoint scripts causes the scripts to fail when run under the default shell, (when not bash). Usage of &> is preferred.

@rhettg
Copy link

rhettg commented Jul 22, 2023

I had this same problem when running as a non-root user:

@rhettg ➜ /workspaces/mcc (rhettg-configure-tailscale) $ /usr/local/sbin/tailscaled-entrypoint
++ id -u
+ [[ 1000 -eq 0 ]]
+ command -v sudo
+ exec
+ sudo --non-interactive sh -c 'mkdir -p /workspaces/.tailscale ; /usr/local/sbin/tailscaled \
    --statedir=/workspaces/.tailscale/ \
    --socket=/var/run/tailscale/tailscaled.sock \
    --port=41641 >& /dev/null'
sh: 4: Syntax error: Bad fd number

Copy link

@noncombatant noncombatant left a comment

Choose a reason for hiding this comment

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

The shebang line calls for bash, and IMO we should not support running bash scripts with other shells. (Who knows what else might break.)

That said, section 3.6.4 of bash's own docs (https://www.gnu.org/software/bash/manual/html_node/Redirections.html) says to prefer the POSIX syntax, so let's do that.

@peterwald
Copy link
Contributor Author

Can someone with write access merge this PR?

@noncombatant noncombatant merged commit 2b05871 into tailscale:main Aug 29, 2023
@peterwald peterwald deleted the pw-entry branch August 29, 2023 20:40
@huw
Copy link
Contributor

huw commented Sep 22, 2023

@noncombatant (or other maintainer) Can you please cut a release for this? ❤️

@noncombatant
Copy link

Oops, sorry about that! Done.

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