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

Fix chamber env and chamber export -f dotenv #385

Merged
merged 7 commits into from
May 12, 2023
Merged

Conversation

mckern
Copy link
Contributor

@mckern mckern commented May 10, 2023

This was reported in #366: chamber made no effort to sanitize keys for either of these commands or formats, which can result in invalid variable names being used for both of these output formats. This PR fixes half of the problem (invalid characters in retrieved keys when rendered via env or export -f dotenv) but does not address the larger question of "what is a valid character when writing a key?"

Some of the construction of these formats have now been unified, with an optional boolean flag for env to preserve key case -- note that keys will still be sanitized, they just won't be converted to uppercase. Since export -f dotenv rewrites values using escape characters, I've made the choice to continue allowing export -f dotenv to always convert keys to uppercase.

New Flags

chamber env has received a couple of new flags: -e, --escape-strings, -p, --preserve-case :

  • --escape-strings will allow chamber env to print values as either safely quoted string literals or safely quoted escape sequences.
  • --preserve-case will force chamber env to respect whatever case a key originally used

Before

$ chamber env -b s3 --backend-s3-bucket dummy-bucket example-service
export EXAMPLE_PASSWORD=cafeface1337
export EXAMPLE-ACCESS-TOKEN=abcdefghijklmnopqrstuvwxyz
export EXAMPLE-USER=itsamemario
export EXAMPLE-MULTILINE='I have eaten
the plums
that were in
the icebox

and which
you were probably
saving
for breakfast

Forgive me
they were delicious
so sweet
and so cold
'

$ chamber export -f dotenv -b s3 --backend-s3-bucket dummy-bucket example-service
EXAMPLE_PASSWORD="cafeface1337"
EXAMPLE-ACCESS-TOKEN="abcdefghijklmnopqrstuvwxyz"
EXAMPLE-USER="itsamemario"
EXAMPLE-MULTILINE="I have eaten\nthe plums\nthat were in\nthe icebox\n\nand which\nyou were probably\nsaving\nfor breakfast\n\nForgive me\nthey were delicious\nso sweet\nand so cold\n'

After

$ chamber env -b s3 --backend-s3-bucket dummy-bucket example-service
export EXAMPLE_PASSWORD=cafeface1337
export EXAMPLE_ACCESS-TOKEN=abcdefghijklmnopqrstuvwxyz
export EXAMPLE_USER=itsamemario
export EXAMPLE_MULTILINE='I have eaten
the plums
that were in
the icebox

and which
you were probably
saving
for breakfast

Forgive me
they were delicious
so sweet
and so cold
'

$ chamber export -f dotenv -b s3 --backend-s3-bucket dummy-bucket example-service
EXAMPLE_PASSWORD="cafeface1337"
EXAMPLE_ACCESS-TOKEN="abcdefghijklmnopqrstuvwxyz"
EXAMPLE_USER="itsamemario"
EXAMPLE_MULTILINE="I have eaten\nthe plums\nthat were in\nthe icebox\n\nand which\nyou were probably\nsaving\nfor breakfast\n\nForgive me\nthey were delicious\nso sweet\nand so cold\n'

What about backwards compatibility?

ce360a34866ff26bd94c2057a9a9b328

Effort has been taken to preserve the spirit of backwards compatibility with the output formats that these commands have rendered in the past. But since the formats were objectively incorrect, I'm really not worried about it.

@mckern mckern requested a review from a team as a code owner May 10, 2023 09:17
This was reported in #366: `chamber` made no effort
to sanitize keys for either of these commands or formats, which can
result in invalid variable names being used for both of these output
formats.

Some of the construction of these formats have now been unified, with
an optional boolean flag for `env` to preserve key case -- note that
keys will still be sanitized, they just won't be converted to
uppercase. Since `export -f dotenv` rewrites values using escape
characters, I've made the choice to continue allowing `export -f
dotenv` to always convert keys to uppercase.
This ensures that `--help` comes after our defined flags, and looks a
little more consistent and professional. Might add it to other flags
in a separate PR but this is a good start.
Copy link
Contributor

@alecjacobs5401 alecjacobs5401 left a comment

Choose a reason for hiding this comment

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

Nice.

Nothing immediately glaring at me, but a few open questions.

Thanks for doing this @mckern! Having this be consistent it amazing

cmd/env.go Show resolved Hide resolved
cmd/env.go Outdated Show resolved Hide resolved
cmd/env_test.go Outdated Show resolved Hide resolved
Also added a note about sort.Strings() being case-sensitive.
Since I switched to using append() for env.sortedKeys() instead of
using an iterator, it's easier to initialize an empty slice and let it
reslice as needed.
Copy link
Contributor

@alecjacobs5401 alecjacobs5401 left a comment

Choose a reason for hiding this comment

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

I have really one comment repeated about a test case for when we're validating/sanitizing shell keys.

I have no strong feelings one way or another and this LGTM as is.

Thanks!

cmd/env_test.go Show resolved Hide resolved
cmd/env_test.go Show resolved Hide resolved
@mckern mckern merged commit e5bc257 into master May 12, 2023
@mckern mckern deleted the mckern/fix-env-vars branch May 12, 2023 01:06
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.

None yet

2 participants