Fix creating and updating hosted deployments with scaling specs #62
Conversation
| } | ||
| obj := HibernationSpecOverride{ | ||
| IsHibernating: types.BoolPointerValue(hibernationOverride.IsHibernating), | ||
| IsActive: types.BoolPointerValue(hibernationOverride.IsActive), |
There was a problem hiding this comment.
Quick note: IsActive is a property set on the API response, it's not something that can be set on the request.
There was a problem hiding this comment.
Yep! got confused a bit by this sometimes but this is for creating the "Terraform response" similar to other "computed" values we have like updatedAt
In resource_deployment.go, we create the request where isActive is not there
| // Currently, the scaling status and spec are only available in development mode | ||
| // However, there is a bug in the API where the scaling status and spec are returned even if the deployment is not in development mode for updated deployments | ||
| // This is a workaround to handle the bug until the API is fixed | ||
| // Issue here: https://github.com/astronomer/astro/issues/21073 |
| @@ -459,11 +441,11 @@ func WorkerQueueDataSourceTypesObject( | |||
| } | |||
|
|
|||
| type DeploymentScalingSpec struct { | |||
| HibernationSpec HibernationSpec `tfsdk:"hibernation_spec"` | |||
There was a problem hiding this comment.
had to change it to use types.Object for resource creation since it would error otherwise
| @@ -282,7 +283,7 @@ func TestAcc_ResourceDeploymentStandard(t *testing.T) { | |||
| ResourceName: azureCeleryResourceVar, | |||
| ImportState: true, | |||
| ImportStateVerify: true, | |||
| ImportStateVerifyIgnore: []string{"external_ips", "oidc_issuer_url", "environment_variables.1.value"}, // environment_variables.0.value is a secret value | |||
| ImportStateVerifyIgnore: []string{"external_ips", "oidc_issuer_url", "scaling_status", "environment_variables.1.value"}, // environment_variables.0.value is a secret value | |||
There was a problem hiding this comment.
Sanity check: ImportStateVerifyIgnore means that TF won't return this in the output of terraform plan or terraform apply? How do these affect the assertions?
There was a problem hiding this comment.
It means that for this test case, we are just ignore those values after an import - we will not check if they are equivalent
Since the first 3 values are computed, it doesnt really matter that the values are not the same and the last value cannot be the same since its a secret
I had it this way since it was throwing errors for those 3
| IsDevelopmentMode: true, | ||
| ScalingSpec: `scaling_spec = {}`, | ||
| }), | ||
| ExpectError: regexp.MustCompile(`Inappropriate value for attribute "scaling_spec"`), |
There was a problem hiding this comment.
nit: invalid over inappropriate is more accurate here
There was a problem hiding this comment.
inappropriate is what the terraform error outputs through their library
| Check: resource.ComposeTestCheckFunc( | ||
| resource.TestCheckResourceAttr(scalingSpecResourceVar, "scaling_spec.hibernation_spec.override.is_hibernating", "true"), | ||
| resource.TestCheckResourceAttr(scalingSpecResourceVar, "scaling_spec.hibernation_spec.override.override_until", "2075-01-01T00:00:00Z"), | ||
| resource.TestCheckNoResourceAttr(scalingSpecResourceVar, "scaling_spec.hibernation_spec.schedules"), |
There was a problem hiding this comment.
nit: for each config, can add comments in assertions on what we're checking - or maybe in the description field something like "Scaling Spec with Hibernation Spec Override Without Schedule" idk if it's overkill, but just an idea for test maintainability
There was a problem hiding this comment.
so I refactored the tests to make it a lil more obvious as to what they do
I think I will skip adding comments just in case we update the tests but forget to update the comments but now the tests should be a lot simpler and readable and the main gist of what is going on should be extremely obvious
Also the tests aren't necessarily for asserting the responses but more importantly to check that it doesnt error or panic when it should succeed
Description
🎟 Issue(s)
#61
🧪 Functional Testing
📸 Screenshots
📋 Checklist
examples/directory