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

Lowercase bool variables values for consistency #16

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

LeoDiazL
Copy link
Contributor

Converted to lower-case and added validations to avoid issues.

@LeoDiazL LeoDiazL linked an issue Nov 11, 2022 that may be closed by this pull request
@arm4b arm4b added the enhancement New feature or request label Nov 11, 2022
Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

Thanks for finding all the places! 👍

The change looks good overall,

Looks like this plugin does something special with the converters, so try to find out the logic there how the conversion applies and at which step. As discussed, please test the helm plugin manually if lowercasing would work OK and doesn't break the normal plugin execution.

@LeoDiazL Please provide your findings in the comments so this PR could be merged.

@arm4b arm4b changed the title Lowercased variables Lowercase bool variables values for consistency Dec 15, 2022
if [[ "${FETCH_KUBECONFIG}" == "False" ]]; then
if [[ "${FETCH_KUBECONFIG}" == "false" ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

From the conversation, this one is still coming uppercased.

The ENV var default is defined in the

helm/bitops.schema.yaml

Lines 69 to 71 in ecac7a1

type: boolean
default: true
export_env: FETCH_KUBECONFIG

The context is that Helm chart is using

core_schema_parsing: false

meaning bool var parsing is happening inside the Helm chart with some bash.

We'll need to ideally find the place where it's happening.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like in the helm pack shyaml tool is used for parsing the plugin schema

k="$(cat $SCHEMA_FILE | shyaml keys $rootkey)"

which probably converts bool values to the uppercase.

Worth the debugging here:

elif [ "$value" == "True" ]; then
OUTPUT="${cli_flag}"

default="$($SCRIPTS_DIR/bitops-config/get.sh $SCHEMA_FILE "${full_value_path_schema}.default")"

Copy link
Member

@arm4b arm4b Dec 15, 2022

Choose a reason for hiding this comment

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

The alternative approach is to throw away all this bash insanity https://github.com/bitops-plugins/helm/blob/main/scripts/bitops-config/convert-schema.sh and switch to (now fixed) core_schema_parsing: true

core_schema_parsing: false

@PhillypHenning
Copy link
Contributor

Screen Shot 2022-12-19 at 3 16 33 PM

Screen Shot 2022-12-19 at 3 16 17 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Updating all boolean values to lowercase
3 participants