Fix broken Terraform installation files #1420
Conversation
|
Hi @josegpg. Thanks for your PR. I'm waiting for a feast-dev member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
infra/terraform/gcp/feast.tf
Outdated
| @@ -113,7 +113,7 @@ resource "helm_release" "feast" { | |||
| depends_on = [kubernetes_secret.feast-postgres-secret, kubernetes_secret.feast_sa_secret] | |||
|
|
|||
| name = var.name_prefix | |||
| chart = "../../charts/feast" | |||
| chart = "feast-charts/feast" | |||
There was a problem hiding this comment.
Would it be easier to use a remote repository?
Something like chart = "https://feast-helm-charts.storage.googleapis.com/feast-0.100.4.tgz"?
There was a problem hiding this comment.
It's easier I agree, with this doc changes are not necessary. However this solution fixes the version and with every new helm chart release you'll have to keep these terraform files up to date.
Tell me what you prefer, I'm good with both
There was a problem hiding this comment.
However this solution fixes the version and with every new helm chart release you'll have to keep these terraform files up to date
I actually think this is a feature, not a bug :)
We'd essentially be pinning the dependencies to a specific version, which I think is ideal. We want to provide an opinionated set of components that we know work well together. So I think pointing to a specific chart makes the most sense.
We could use this argument layout to specific the repo + chart + version
resource "helm_release" "example" {
name = "my-redis-release"
repository = "https://charts.bitnami.com/bitnami"
chart = "redis"
version = "6.0.1"
}
There was a problem hiding this comment.
It makes sense! Thanks for explaining it. I've already tested with your suggested change and it works. I'll update the PR
|
Thanks for this @josegpg! |
woop
changed the title
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: josegpg, woop The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
This PR only fixes small issues with Terraform reference configuration for GCP so the Quickstart guide actually works.
Which issue(s) this PR fixes:
Fixes #1419
Does this PR introduce a user-facing change?: