Add importer for helm release resource #150
Conversation
|
Related to #113 |
|
Didn’t find the time yet to review the code. On the use of |
|
Oh good idea. A repository can have |
|
How about doing repo.release? Then use the last dot? |
|
I updated the pr to have the suggested changes: using |
|
I verified that PR and the importer works fine. Thank you @Lemmons ! then the resource should be defined in the terraform code like this: # Works OK
resource "helm_release" "my_mariadb" {
name = "my_mariadb"
chart = "mariadb"
repository = "stable"
namespace = "test_namespace"
# <...>
}But it will not work properly if you put repository name as part of the chart name (as recommended on the # Doesn't work OK
resource "helm_release" "my_mariadb" {
name = "my_mariadb"
chart = "stable/mariadb"
namespace = "test_namespace"
# <...>
}In the second case after the import, terraform plan will show changes of both |
# terraform.tf
resource "helm_repository" "stable" {
name = "stable"
url = "https://kubernetes-charts.storage.googleapis.com"
}
resource "helm_release" "test-nginx-ingress" {
namespace = "test"
name = "test-nginx-ingress"
repository = "stable"
chart = "nginx-ingress"
version = "0.28.1"
values = ["${file("${path.module}/input_values.yaml")}"]
}
ResultReasonIt seems that |
|
Dang. Good catch. I'll see if I can carve out some time to fix this in the next week or so. |
|
Also, regarding your previous comment, @legal90, when I'm working on the fix for the type issue, I'll see if I can find a way to support including the repo in the chart name. I started off doing it that way, and decided against it at some point, but I don't clearly remember why now. I think there might be a way to support it, but it also might require a fairly significant refactor of other logic in the provider not directly related to the import. If that is the case, I will hold off on those changes and do them as a subsequent pr to keep the complexity of this pr as low as possible. |
|
@Lemmons Thank you! Yeah, the first thing is just a limitation and could be solved separately (I guess so). But I also found that the importer doesn't respect existing I thought about this today and came up with a PoC solution (which actually also fixes the crash mentioned above). It's in the commit of the fork of your branch: It adds a new attribute Maybe we can reuse |
The new "overrides" attribute saved the compiled version of all "values" + "set" + "set_string" values merged. It is assumed that "overrides" will be the only attribute showing the diff of these values. Diffs of "values", "set" and "set_string" attributes are suppressed.
Update the release only if any of values, chart version or reuse_values are changed, OR any of recreate_pods or force_update are set to true
|
Pinging @Lemmons @meyskens I believe that will help a lot with an existing workflow of this provider and will improve the user experience in terms of values definition. Let me know if I should send it as an additional pull-request. Cheers! ;) |
Add overrides to support better diff logic
|
@legal90 Thanks for the work and the ping. I definitely think this is the correct way to go. I've merged your changes into this pr. |
|
@meyskens any thoughts about the path to get this merged? I will deal with the merge conflicts in the next few days, but beyond that, what else should we do before we can move forward on this? |
|
Since #153 just got merged, we might need to wrap the newly introduced Since that part (with |
|
I moved the implementation of Hopefully, it will make both PRs easier to review & merge. |
|
Looks like this PR is stale now. Is there any plan to move forward here? |
|
Is there any update on this PR? |
|
can I help with this in any way? |
|
Unfortunately I haven't had time to pick this back up. Closing this in favor of #337 |
ghost
locked and limited conversation to collaborators
This pr adds the ability to import helm release resources.
It takes the release name and repository as the import id and currently is using
---as the delineator between the two. I chose---simply because it provides good separation between the two pieces of the import id and is not very likely to be a part of either. It is, admittedly, quite naive to assume nobody is going to use---in their release or repository names, so if anyone has a better idea there it would be appreciated. Another solution would be to use a single character, such as-and then escape any-s in either part of the id, but this would mean adding a lot more logic to the import id parsing, and would make it much more confusing to import release with a-in them.It then gets the values out of the release and stores them as
sets in state. This means that if you define your release in terraform usingvalues, or a combination ofvaluesandsets, your first plan will show an in-place modification of the release, though likely it will result in no changes in helm. I went back and forth on which one to use,setorvalues, but in the end, either will lead to the same issue, because, as far as I can tell, Helm stores everything as a combined list of values, which means there is no way to distinguish between the two once a release is out there.I'm expecting a bit of feedback, but at least wanted to get this out there so we can start a discussion on the best way to implement this.