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 double quotes surrounding a wildcard #2165

Merged
merged 2 commits into from
Feb 8, 2023
Merged

Remove double quotes surrounding a wildcard #2165

merged 2 commits into from
Feb 8, 2023

Conversation

spgreenberg
Copy link
Contributor

From man bash: Enclosing characters in double quotes preserves the literal value of all characters within the quotes, with the exception of $, `, \

In bash, this should not work. We would like to be able to point a wide range of users to the deploy-on-kind.sh script which calls this script. We would like to ensure the widest range of users do not run into issue with this.

Is there a related GitHub Issue?

No

What is this change about?

The script uses a wildcard inside double quotes. In bash, this should not work. We would like to be able to point a wide range of users to the deploy-on-kind.sh script which calls this script. We would like to ensure the widest range of users do not run into issue with this.

Does this PR introduce a breaking change?

No

Acceptance Steps

Run it in vanilla bash with no special settings.

Tag your pair, your PM, and/or team

N/A

From the `man bash`: Enclosing characters in double quotes preserves the literal value of all characters within the quotes, with the exception of $, `, \
Copy link
Contributor

@kieron-dev kieron-dev left a comment

Choose a reason for hiding this comment

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

Removing the quotes breaks the case where $VENDOR_DIR contains spaces. Quoting just "$VENDOR_DIR" would be ok.

Are you seeing this problem on a mac with the default bash-v3.2 btw?

@gcapizzi
Copy link
Contributor

gcapizzi commented Feb 7, 2023

Also note that kubectl apply -f will only accept one file regardless, so this will only work if the wildcard expands to exactly one file. How we quote it shouldn't matter, but we could change this to be targeting a single file more explicitly? Anything I can think of looks worse than what we already have though.

@spgreenberg
Copy link
Contributor Author

I see this issue on osx with bash 5.2. I can revise to move the quotes.

@spgreenberg
Copy link
Contributor Author

The latest commit should expand the wildcard and handle spaces in the path.

@gcapizzi vanilla bash doesn't expand wildcards in double quotes per the man page quote above. It isn't working as is with plain bash 5.2 on macos. The script is so quick and easy that I want to be able to point as many people to it as possible.

@danail-branekov
Copy link
Member

The script is so quick and easy that I want to be able to point as many people to it as possible

Keep in mind that we do not "release" this script, we use it for dev purposes and will most probably change in future. I agree that such a script is much better than having an outdated documentation but we cannot guarantee that it works at all times.

@spgreenberg
Copy link
Contributor Author

Understood. Given it is used for dev, I am sure it will get plenty of attention. We can make it clear this is a dev script and it should be treated as such.

@gcapizzi
Copy link
Contributor

gcapizzi commented Feb 7, 2023

@spgreenberg We do not consider the script to be public API and we will change it for development purposes without warning, so we don't want people to think of it as something they can rely on. It will always work (as we use it daily) but both the behaviour and the interface might change.

We do try our best to make sure that install instructions in our releases are correct, those combined with the Helm chart are aimed at users.

@spgreenberg
Copy link
Contributor Author

@gcapizzi I understand. Currently this is the best option available though. We are working on a streamlined introduction to Korifi and we want to be sure the experience is as easy as possible. We will be sure to stress that this is a development tool and that Korifi is still actively being built.

@kieron-dev kieron-dev enabled auto-merge (rebase) February 8, 2023 13:54
@kieron-dev kieron-dev merged commit a50ead5 into cloudfoundry:main Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants