-
Notifications
You must be signed in to change notification settings - Fork 89
Add support for defining encryption key #11
Add support for defining encryption key #11
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for submitting this PR! As it's been about 4 weeks since we're responding, I'll go ahead and commit these changes to your branch as a PR there so you can easily add them here.
modules/run-consul/run-consul
Outdated
@@ -150,6 +151,7 @@ function generate_consul_config { | |||
local readonly user="$4" | |||
local readonly cluster_tag_name="$5" | |||
local readonly cluster_size_instance_metadata_key_name="$6" | |||
local readonly encrypt="$7" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use a var name like encrypt_key
so users don't confuse this with a boolean.
modules/run-consul/run-consul
Outdated
@@ -341,7 +354,8 @@ function run { | |||
"$config_dir" \ | |||
"$user" \ | |||
"$cluster_tag_name" \ | |||
"$CLUSTER_SIZE_INSTANCE_METADATA_KEY_NAME" | |||
"$CLUSTER_SIZE_INSTANCE_METADATA_KEY_NAME" \ | |||
"$encrypt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern with this line is that if we add any additional vars in the future and $encrypt
is empty, the Bash function will ignore $encrypt
entirely and the function arguments won't be passed to the correct argument position. Therefore, we need a null
equivalent in Bash, but since Bash doesn't give us one, we should define a readonly
var at the top like this:
readonly EMPTY_VAL="__EMPTY__"
Then we replace
local encrypt=""
with
local encrypt="$EMPTY_VAL"
Finally, we can update the code as follows:
# old
local encrypt_config=""
if [[ "$encrypt" != "" ]]; then
encrypt_config="\"encrypt\": \"$encrypt\","
fi
...
"ui": true,
$encrypt_config,
...
# new
if [[ "$encrypt" == "$EMPTY_VAL" ]]; then
encrypt=""
fi
...
"ui": true,
"encrypt": "$encrypt",
...
Alright, just uploaded the changes and manually validated so we're good now. Merging! |
Adds a CLI flag for defining encryption keys to Consul configuration. This is a non-breaking change as
encrypt
key uses an empty string as a default value.