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

[Bug Fix] Support exporting environment variables with whitespaces #7

Merged
merged 4 commits into from
Aug 20, 2024

Conversation

ManuelPalenzuelaDD
Copy link
Contributor

@ManuelPalenzuelaDD ManuelPalenzuelaDD commented Aug 7, 2024

@### What does this PR do

Previously, when attempting to export environment variables with spaces (e.g. NODE_OPTIONS="-r [...]"), we would get an error message: (e.g. export: not valid in this context: -r). This is because xargs doesn't properly support strings with spaces.

To fix this, we instead advise to export the env variables with a different approach that supports values with spaces.

Possible Drawbacks / Trade-offs

It is a bit more obscure to see what the script is doing.

Describe how to test/QA your changes

Run the script with DD_CIVISIBILITY_INSTRUMENTATION_LANGUAGES=js and see how the NODE_OPTIONS environment variable is now exported correctly (before this change an error was thrown).

Previously, when attemping to export enviornment variables with spaces
(e.g. NODE_OPTIONS="-r [...]"), we would get an error message: `export: not valid in this context: -r`.
This is because xargs doesn't properly support strings with spaces.

To fix this, we are doing two things:
1. Switch to doing an `eval` of the script's output (which exports the
   env vars)
2. Properly escape the environment variables.

This results in us being able to export environment variables with
spaces.
README.md Outdated
@@ -38,5 +38,5 @@ JAVA_TOOL_OPTIONS=-javaagent:./.datadog/dd-java-agent.jar

If you want to set the variables printed by the script, use the following expression:
```shell
export $(DD_CIVISIBILITY_INSTRUMENTATION_LANGUAGES=... DD_API_KEY=... ./install_test_visibility.sh | xargs)
eval $(DD_CIVISIBILITY_INSTRUMENTATION_LANGUAGES=... DD_API_KEY=... ./install_test_visibility.sh | sed 's/^/export /')

Choose a reason for hiding this comment

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

@ManuelPalenzuelaDD I would rather make the script print the export statements directly (like, for example, ssh-agent -s does), instead of adding the exports with sed to the script output. The sed does not really ensure we only do exports, because you can still emit multiple commands in a single line separated by ;, you can still emit $(arbitrary_command), etc.

Choose a reason for hiding this comment

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

Also, I would quote the output here, like:

Suggested change
eval $(DD_CIVISIBILITY_INSTRUMENTATION_LANGUAGES=... DD_API_KEY=... ./install_test_visibility.sh | sed 's/^/export /')
eval "$(DD_CIVISIBILITY_INSTRUMENTATION_LANGUAGES=... DD_API_KEY=... ./install_test_visibility.sh | sed 's/^/export /')"

Otherwise you can still have problems with spaces (multiple spaces will be turned into one, wildcards in the output may be expanded, etc.).

Copy link
Contributor Author

@ManuelPalenzuelaDD ManuelPalenzuelaDD Aug 8, 2024

Choose a reason for hiding this comment

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

Yep, I agree. If we do this though we'll need to change the logic of the existing GitHub action to remove the exports from the output (as it pipes them to a file in the format of KEY=VAL). I'll wait to speak to @nikita-tkachenko-datadog about this :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Having the script printing export commands and later removing them in the Github action seems like a weird approach to me.
It was not the intention to print something that can be directly evaled, but rather to have something as simple as NAME=VALUE that every user of the script can process easily to fit their needs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am also wary of using eval, even though we verify the integrity of the script with checksum.
Could we use something more restrictive?
For example something along these lines

while IFS='=' read -r name value; do
  if [[ $name =~ ^[A-Za-z_][A-Za-z0-9_]*$ ]]; then
    export "$name=$value"
  fi
done < <(./install_test_visibility.sh)

Copy link
Contributor Author

@ManuelPalenzuelaDD ManuelPalenzuelaDD Aug 19, 2024

Choose a reason for hiding this comment

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

@nikita-tkachenko-datadog Yes, I explored doing something like this but decided for the eval approach due to it being more readable for customers (as some may copy paste our instructions from this readme). If we think that the readability problem is a non issue then lets go with this :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't have a strong opinion wrt eval vs ad-hoc parsing. I am fine with either approach as long as it does not imply adding things to the output to remove them later :)

Copy link
Contributor Author

@ManuelPalenzuelaDD ManuelPalenzuelaDD Aug 19, 2024

Choose a reason for hiding this comment

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

👌 Lets go with the more restrictive approach for now as I don't think there are any customers using this script directly. If readability starts being a problem for customers we can evaluate later whether we should change to the alternative eval approach.

@ManuelPalenzuelaDD ManuelPalenzuelaDD merged commit 9798acf into main Aug 20, 2024
1 check passed
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.

3 participants