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

Git operator values #306

Merged
merged 1 commit into from
Sep 21, 2021
Merged

Git operator values #306

merged 1 commit into from
Sep 21, 2021

Conversation

msvticket
Copy link
Member

@msvticket msvticket commented Sep 13, 2021

Description

Make it possible to add helm value to the jx-git-operator helm chart

Which issue this PR fixes

fixes #303

@@ -7,6 +7,8 @@ resource "helm_release" "jx-git-operator" {
version = "0.0.194"
create_namespace = true

values = var.jx_git_operator_values
Copy link
Member

@ankitm123 ankitm123 Sep 16, 2021

Choose a reason for hiding this comment

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

Instead of this, we could use

values = [
    "${file("values.yaml")}"
]

where the user can have a values.yaml (probably need to modify the value file location like we do for the nginx module) in their module directory.
Example: https://registry.terraform.io/providers/hashicorp/helm/latest/docs/resources/release#example-usage---chart-repository
IMHO, the values file is a more native approach to setting values in the helm chart, and we should follow that, wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a bit confusing, since in the mail with the review the text was this:

Instead of this, we could use

values = [
"${file("values.yaml")}"
]
Example: https://registry.terraform.io/providers/hashicorp/helm/latest/docs/resources/release#example-usage---chart-repository
For now, this is ok.

So I wondered why the PR wasn't merged...

I looked at the nginx module and I think that solution is both more complicated and give a worse user experience. Typically you only want to set few values. With my solution that can then be done inline in the tf file. If the user wants to put it in a file they case do so and use the file function themselves. Also the nginx solution means that even if the user only want to add one value he still needs to add a file with all the default values.

@ankitm123 ankitm123 merged commit b27a316 into jenkins-x:master Sep 21, 2021
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.

Support supplying values to git-operator chart
2 participants