hashicorp / terraform-k8s

Terraform Cloud Operator for Kubernetes
https://learn.hashicorp.com/tutorials/terraform/kubernetes-operator?in=terraform/kubernetes
Mozilla Public License 2.0
455 stars 71 forks source link

Fix bug with timing of configuration version status #78

Closed mcfearsome closed 3 years ago

mcfearsome commented 3 years ago

Community Note

Closes #20

Release note for CHANGELOG:

* Fix bug where the configuration version in TFC isn't in an "uploaded" status when the run is started
mcfearsome commented 3 years ago

I left out my changes to CreateRun as it doesn't seem to be in use.

mcfearsome commented 3 years ago

Thank you @koikonom for the review! Your suggestions make sense, using a sleep is generally something I try to avoid in any language but I took your code as permission to do so 😁 not to mention the added complication and the fact I was focused on getting a working operator for other work I was trying to complete (this operator is on its way to becoming a core part of our infra and I just put it into a production cluster for the first time 😬). I believe I will have to store the configuration version in the status now so that I can check the status before uploading. I should be able to rework this over the next day or so.

mcfearsome commented 3 years ago

Thinking through this a little more I'm trying to determine how to know when I need to upload a new config version. It seems to me that I would have to pull down the actual uploaded config (a quick read of the api docs doesn't appear to show a clear way of getting this) so I can compare, or possibly using an intermediary state in the status to let me know the config has been uploaded but not run. Forgive me if I'm missing something obvious, I'm on mobile and haven't had a chance to really read through everything yet.

koikonom commented 3 years ago

Yeah that function should have been removed when I moved things around, but I missed it :) Sorry for that.

As for your other question, here are a few suggestions:

Hope these help.

mcfearsome commented 3 years ago

I'm sure we will move to VCS backed repos eventually, but for now we have multiple workspaces spread across 60 some clusters and it will take some time to migrate them over while trying to juggle lots of other tasks. For now however I think it makes sense to maintain backwards compatibility, at least for a bit, and have the operator actually work properly (without this change it required a lot of manual intervention but has been a dream since). For now my plan is to add the configuration version id to the status and if that is present skipping the upload, then removing the value once the run has triggered.

koikonom commented 3 years ago

You only need to upload a new Configuration Version if you trigger a new run, and the only way to trigger a new run is to either change the module version, or if someone changes the variables on the workspace.

mcfearsome commented 3 years ago

I have re-worked this to utilize the built-in Requeue mechanism and even included a test! I am still new to Golang and this is the first real test I've written I think so please let me know if there are things I could do better.

koikonom commented 3 years ago

Hi @mcfearsome, thanks for working on this. Sorry I haven't responded to it yet, but it's definitely on my to-do list.