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

remove unnecessary semicolon which breaks zsh on mac #16185

Merged
merged 2 commits into from
Feb 3, 2025

Conversation

craigbox
Copy link
Contributor

zsh, in many default configurations, escapes special characters on paste. The ; in the Gateway API example is replaced with \; which causes the command to fail.

As it's not a load-bearing semi-colon, lets just remove it.

Fixes #16162

@craigbox craigbox requested review from a team as code owners January 20, 2025 08:40
@istio-testing istio-testing added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 20, 2025
@craigbox
Copy link
Contributor Author

you're kidding me. it is a load bearing semi-colon?

@craigbox
Copy link
Contributor Author

Now we don't use kustomize should I just nuke the braces?

@kfaseela
Copy link
Member

saw the lint failure only after approving :D

@craigbox
Copy link
Contributor Author

note to self, make Faseela an approver for Ukranian too

@bleggett
Copy link
Contributor

you're kidding me. it is a load bearing semi-colon?

It's required for bash, we are using shellcheck in bash syntax mode, and the snippet is bash syntax.

The actual way to handle stuff like this is have a syntax=sh (POSIX sh subset only) and have shellcheck validate POSIX sh rules versus defaulting to bash syntax (which lots of shells emulate, but not exactly).

POSIX sh is the actual "common" subset to target if you want stuff that runs in any shell.

@craigbox
Copy link
Contributor Author

I don't think it's "required for bash" in the sense that things work if you remove it, but only because the only remaining command is a }, which is treated as a literal.

I also think our shellcheck scripts aren't smart enough to distinguish between POSIX sh and bash at this point?

Should we go for the trivial fix of changing the command?

kubectl get crd gateways.gateway.networking.k8s.io &> /dev/null || kubectl apply -f https://github.com/kubernetes-sigs/gateway-api/releases/download/v1.2.0/standard-install.yaml

...seems like it will do what we want?

@ilrudie
Copy link
Contributor

ilrudie commented Jan 29, 2025

yeah, It seems to me that something like this should work

{{< text syntax=bash snip_id=install_crds >}}
$ kubectl get crd gateways.gateway.networking.k8s.io &> /dev/null || \
kubectl apply -f https://github.com/kubernetes-sigs/gateway-api/releases/download/{{< k8s_gateway_api_version >}}/standard-install.yaml
{{< /text >}}

@bleggett
Copy link
Contributor

bleggett commented Jan 29, 2025

I don't think it's "required for bash" in the sense that things work if you remove it, but only because the only remaining command is a }, which is treated as a literal.

Right - my point is that we're doing bash-centric linting and CI, so we should expect breakage for non-bash, because bash the subdialect isn't really a standard outside of bash the shell.

I also think our shellcheck scripts aren't smart enough to distinguish between POSIX sh and bash at this point?

Fair enough - shellcheck definitely can distinguish, but our stuff wrapping it may not be able to.

@istio-testing istio-testing merged commit 9d971f6 into istio:master Feb 3, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix our install doc on gw API
5 participants