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

add helmStringValues support #1180

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

faust2199
Copy link

Fixes #1179

Copy link
Member

@gerardcl gerardcl left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

Copy link
Contributor

@jafarre-bi jafarre-bi left a comment

Choose a reason for hiding this comment

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

Do we need both values and string values? We are using strings in both cases. Not sure about its usefulness.
Apart from that, this is a new parameter that we need to reflect in the TIR. The changes to the TIR are about to be merged. When that is ready, merge those changes into this branch and add the support for this new parameter.

Copy link
Contributor

@jafarre-bi jafarre-bi left a comment

Choose a reason for hiding this comment

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

Please wait for the changes to the TIR which are about to be merged and then update the TIR with this new parameter.

@faust2199
Copy link
Author

Do we need both values and string values? We are using strings in both cases. Not sure about its usefulness. Apart from that, this is a new parameter that we need to reflect in the TIR. The changes to the TIR are about to be merged. When that is ready, merge those changes into this branch and add the support for this new parameter.

It is possible that someone has been using existing automatic type conversion. A hypothetical example:

  odsComponentStageRolloutOpenShiftDeployment(context, [
    'selector': "app.kubernetes.io/name=${context.componentId}",
    'helmEnvBasedValuesFiles': ["values.env.yaml"],
    'helmValues': [
      'replicaCount': '3',
    ],
  ])

The above works, because it becomes --set replicaCount=3, then 3 becomes integer. If we make it always --set-string, then it will not work. Of course, it is also possible that no one has been using it this way and we don't have to maintain backwards compatibility.

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.

Helm chart validation error if a commit hash starts with 8 or more numbers
3 participants