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

TimestreamInfluxDB DB instance add more updatable arguments #7

Conversation

trevorbonas
Copy link

@trevorbonas trevorbonas commented Dec 17, 2024

Description

  • network_type added as an argument.
  • port added as an argument.
  • deployment_type is now updatable.
    • An update step has been added to TestAccTimestreamInfluxDBDBInstance_deploymentType.
  • db_instance_type is now updatable.
    • The test TestAccTimestreamInfluxDBDBInstance_dbInstanceType has been added with an update step.
  • flex.Expand is no longer used in Update to populate an UpdateDbInstanceInput struct. This is because a ValidationException will occur when a DB instance is updated with configuration values it already uses.
  • In Update, if secondary_availability_zone is left as unknown, it is set to its previous value. secondary_availability_zone can be left as unknown when tags are updated, which causes an error.
  • secondary_availability_zone uses the default value of nil. While unconventional, this prevents an error when updating deployment_type. secondary_availability_zone's value is directly a result of the value of deployment_type.
  • awstypes.StatusDeleted has been added as a pending state in waitDBInstanceDeleted. This is because DB instances are first given the status "DELETED" before becoming unreachable. This status is not consistent. Using this status as a target status leads to flaky tests. Using this status as a pending status means Terraform will wait until the instance is unreachable.
  • An error in the website documentation for creating an instance with S3 log delivery configuration has been fixed.

Relations

N/A.

References

Updated UpdateDbInstanceInput struct documentation.

Output from Acceptance Testing

% make testacc TESTS=TestAccTimestreamInfluxDBDBInstance PKG=timestreaminfluxdb ACCTEST_PARALLELISM=1 ACCTEST_TIMEOUT=1440m
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.23.3 test ./internal/service/timestreaminfluxdb/... -v -count 1 -parallel 1 -run='TestAccTimestreamInfluxDBDBInstance'  -timeout 1440m
2024/12/19 14:48:35 Initializing Terraform AWS Provider...
=== RUN   TestAccTimestreamInfluxDBDBInstance_tags
=== PAUSE TestAccTimestreamInfluxDBDBInstance_tags
=== RUN   TestAccTimestreamInfluxDBDBInstance_tags_null
=== PAUSE TestAccTimestreamInfluxDBDBInstance_tags_null
=== RUN   TestAccTimestreamInfluxDBDBInstance_tags_EmptyMap
=== PAUSE TestAccTimestreamInfluxDBDBInstance_tags_EmptyMap
=== RUN   TestAccTimestreamInfluxDBDBInstance_tags_AddOnUpdate
=== PAUSE TestAccTimestreamInfluxDBDBInstance_tags_AddOnUpdate
=== RUN   TestAccTimestreamInfluxDBDBInstance_tags_EmptyTag_OnCreate
=== PAUSE TestAccTimestreamInfluxDBDBInstance_tags_EmptyTag_OnCreate
=== RUN   TestAccTimestreamInfluxDBDBInstance_tags_EmptyTag_OnUpdate_Add
=== PAUSE TestAccTimestreamInfluxDBDBInstance_tags_EmptyTag_OnUpdate_Add
=== RUN   TestAccTimestreamInfluxDBDBInstance_tags_EmptyTag_OnUpdate_Replace
=== PAUSE TestAccTimestreamInfluxDBDBInstance_tags_EmptyTag_OnUpdate_Replace
=== RUN   TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_providerOnly
=== PAUSE TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_providerOnly
=== RUN   TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_nonOverlapping
=== PAUSE TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_nonOverlapping
=== RUN   TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_overlapping
=== PAUSE TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_overlapping
=== RUN   TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_updateToProviderOnly
=== PAUSE TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_updateToProviderOnly
=== RUN   TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_updateToResourceOnly
=== PAUSE TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_updateToResourceOnly
=== RUN   TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_emptyResourceTag
=== PAUSE TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_emptyResourceTag
=== RUN   TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_emptyProviderOnlyTag
=== PAUSE TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_emptyProviderOnlyTag
=== RUN   TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_nullOverlappingResourceTag
=== PAUSE TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_nullOverlappingResourceTag
=== RUN   TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_nullNonOverlappingResourceTag
=== PAUSE TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_nullNonOverlappingResourceTag
=== RUN   TestAccTimestreamInfluxDBDBInstance_tags_ComputedTag_OnCreate
=== PAUSE TestAccTimestreamInfluxDBDBInstance_tags_ComputedTag_OnCreate
=== RUN   TestAccTimestreamInfluxDBDBInstance_tags_ComputedTag_OnUpdate_Add
=== PAUSE TestAccTimestreamInfluxDBDBInstance_tags_ComputedTag_OnUpdate_Add
=== RUN   TestAccTimestreamInfluxDBDBInstance_tags_ComputedTag_OnUpdate_Replace
=== PAUSE TestAccTimestreamInfluxDBDBInstance_tags_ComputedTag_OnUpdate_Replace
=== RUN   TestAccTimestreamInfluxDBDBInstance_tags_IgnoreTags_Overlap_DefaultTag
=== PAUSE TestAccTimestreamInfluxDBDBInstance_tags_IgnoreTags_Overlap_DefaultTag
=== RUN   TestAccTimestreamInfluxDBDBInstance_tags_IgnoreTags_Overlap_ResourceTag
=== PAUSE TestAccTimestreamInfluxDBDBInstance_tags_IgnoreTags_Overlap_ResourceTag
=== RUN   TestAccTimestreamInfluxDBDBInstance_basic
=== PAUSE TestAccTimestreamInfluxDBDBInstance_basic
=== RUN   TestAccTimestreamInfluxDBDBInstance_disappears
=== PAUSE TestAccTimestreamInfluxDBDBInstance_disappears
=== RUN   TestAccTimestreamInfluxDBDBInstance_dbInstanceType
=== PAUSE TestAccTimestreamInfluxDBDBInstance_dbInstanceType
=== RUN   TestAccTimestreamInfluxDBDBInstance_logDeliveryConfiguration
=== PAUSE TestAccTimestreamInfluxDBDBInstance_logDeliveryConfiguration
=== RUN   TestAccTimestreamInfluxDBDBInstance_publiclyAccessible
=== PAUSE TestAccTimestreamInfluxDBDBInstance_publiclyAccessible
=== RUN   TestAccTimestreamInfluxDBDBInstance_deploymentType
=== PAUSE TestAccTimestreamInfluxDBDBInstance_deploymentType
=== CONT  TestAccTimestreamInfluxDBDBInstance_tags
--- PASS: TestAccTimestreamInfluxDBDBInstance_tags (1241.54s)
=== CONT  TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_nullOverlappingResourceTag
--- PASS: TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_nullOverlappingResourceTag (1038.44s)
=== CONT  TestAccTimestreamInfluxDBDBInstance_deploymentType
--- PASS: TestAccTimestreamInfluxDBDBInstance_deploymentType (1258.41s)
=== CONT  TestAccTimestreamInfluxDBDBInstance_publiclyAccessible
--- PASS: TestAccTimestreamInfluxDBDBInstance_publiclyAccessible (1204.51s)
=== CONT  TestAccTimestreamInfluxDBDBInstance_logDeliveryConfiguration
--- PASS: TestAccTimestreamInfluxDBDBInstance_logDeliveryConfiguration (1242.03s)
=== CONT  TestAccTimestreamInfluxDBDBInstance_dbInstanceType
--- PASS: TestAccTimestreamInfluxDBDBInstance_dbInstanceType (1797.53s)
=== CONT  TestAccTimestreamInfluxDBDBInstance_disappears
--- PASS: TestAccTimestreamInfluxDBDBInstance_disappears (1064.48s)
=== CONT  TestAccTimestreamInfluxDBDBInstance_basic
--- PASS: TestAccTimestreamInfluxDBDBInstance_basic (1005.65s)
=== CONT  TestAccTimestreamInfluxDBDBInstance_tags_IgnoreTags_Overlap_ResourceTag
--- PASS: TestAccTimestreamInfluxDBDBInstance_tags_IgnoreTags_Overlap_ResourceTag (1104.66s)
=== CONT  TestAccTimestreamInfluxDBDBInstance_tags_IgnoreTags_Overlap_DefaultTag
--- PASS: TestAccTimestreamInfluxDBDBInstance_tags_IgnoreTags_Overlap_DefaultTag (1153.00s)
=== CONT  TestAccTimestreamInfluxDBDBInstance_tags_ComputedTag_OnUpdate_Replace
--- PASS: TestAccTimestreamInfluxDBDBInstance_tags_ComputedTag_OnUpdate_Replace (1168.39s)
=== CONT  TestAccTimestreamInfluxDBDBInstance_tags_ComputedTag_OnUpdate_Add
--- PASS: TestAccTimestreamInfluxDBDBInstance_tags_ComputedTag_OnUpdate_Add (1010.84s)
=== CONT  TestAccTimestreamInfluxDBDBInstance_tags_ComputedTag_OnCreate
--- PASS: TestAccTimestreamInfluxDBDBInstance_tags_ComputedTag_OnCreate (928.07s)
=== CONT  TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_nullNonOverlappingResourceTag
--- PASS: TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_nullNonOverlappingResourceTag (1006.42s)
=== CONT  TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_providerOnly
--- PASS: TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_providerOnly (1120.06s)
=== CONT  TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_emptyProviderOnlyTag
--- PASS: TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_emptyProviderOnlyTag (971.22s)
=== CONT  TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_emptyResourceTag
--- PASS: TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_emptyResourceTag (1214.60s)
=== CONT  TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_updateToResourceOnly
--- PASS: TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_updateToResourceOnly (868.35s)
=== CONT  TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_updateToProviderOnly
--- PASS: TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_updateToProviderOnly (913.36s)
=== CONT  TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_overlapping
--- PASS: TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_overlapping (1078.49s)
=== CONT  TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_nonOverlapping
--- PASS: TestAccTimestreamInfluxDBDBInstance_tags_DefaultTags_nonOverlapping (1086.47s)
=== CONT  TestAccTimestreamInfluxDBDBInstance_tags_EmptyTag_OnCreate
--- PASS: TestAccTimestreamInfluxDBDBInstance_tags_EmptyTag_OnCreate (1026.52s)
=== CONT  TestAccTimestreamInfluxDBDBInstance_tags_EmptyTag_OnUpdate_Replace
--- PASS: TestAccTimestreamInfluxDBDBInstance_tags_EmptyTag_OnUpdate_Replace (1093.49s)
=== CONT  TestAccTimestreamInfluxDBDBInstance_tags_EmptyTag_OnUpdate_Add
--- PASS: TestAccTimestreamInfluxDBDBInstance_tags_EmptyTag_OnUpdate_Add (1119.81s)
=== CONT  TestAccTimestreamInfluxDBDBInstance_tags_EmptyMap
--- PASS: TestAccTimestreamInfluxDBDBInstance_tags_EmptyMap (1072.82s)
=== CONT  TestAccTimestreamInfluxDBDBInstance_tags_AddOnUpdate
--- PASS: TestAccTimestreamInfluxDBDBInstance_tags_AddOnUpdate (1116.79s)
=== CONT  TestAccTimestreamInfluxDBDBInstance_tags_null
--- PASS: TestAccTimestreamInfluxDBDBInstance_tags_null (977.68s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/timestreaminfluxdb	29888.714s

Copy link

Community Note

Voting for Prioritization

  • Please vote on this pull request by adding a 👍 reaction to the original post to help the community and maintainers prioritize this pull request.
  • Please see our prioritization guide for information on how we prioritize.
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.

For Submitters

  • Review the contribution guide relating to the type of change you are making to ensure all of the necessary steps have been taken.
  • For new resources and data sources, use skaff to generate scaffolding with comments detailing common expectations.
  • Whether or not the branch has been rebased will not impact prioritization, but doing so is always a welcome surprise.

@trevorbonas
Copy link
Author

trevorbonas commented Dec 17, 2024

Tests are currently running, they will take around 10 hours to run. The last time a test failed was because of the status "DELETED". I haven't been able to reproduce this test failure since adding awstypes.StatusDeleted as a pending target, but then, again, it was an inconsistent error to reproduce in the first place. We will see if tests succeed.

After this PR is reviewed, I will close it and create an upstream PR.

@@ -494,6 +519,10 @@ func (r *resourceDBInstance) Update(ctx context.Context, req resource.UpdateRequ
plan.SecondaryAvailabilityZone = flex.StringToFrameworkLegacy(ctx, output.SecondaryAvailabilityZone)
}

if plan.SecondaryAvailabilityZone.IsUnknown() {
plan.SecondaryAvailabilityZone = state.SecondaryAvailabilityZone
Copy link
Author

@trevorbonas trevorbonas Dec 17, 2024

Choose a reason for hiding this comment

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

This is the fix for secondary_availability_zone being left as unknown when tags are updated. If the new value (in plan) is unknown, set it to its old value (in state).

From what I can determine, the problem is that Update is called when tags are updated (for some reason) but UpdateDbInstance isn't actually called, as UpdateDbInstance doesn't allow tags to be updated, therefore secondary_availability_zone is left as unknown. I would like a better fix that scaled better for possible future computed values, but at this point I'm not sure what that better fix would be.

Choose a reason for hiding this comment

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

might be good to add a small comment regarding this in the code 👍

Copy link
Author

Choose a reason for hiding this comment

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

Added.

Copy link

@fredjoonpark fredjoonpark left a comment

Choose a reason for hiding this comment

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

Yay, looks good! 🙌

@@ -494,6 +519,10 @@ func (r *resourceDBInstance) Update(ctx context.Context, req resource.UpdateRequ
plan.SecondaryAvailabilityZone = flex.StringToFrameworkLegacy(ctx, output.SecondaryAvailabilityZone)
}

if plan.SecondaryAvailabilityZone.IsUnknown() {
plan.SecondaryAvailabilityZone = state.SecondaryAvailabilityZone

Choose a reason for hiding this comment

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

might be good to add a small comment regarding this in the code 👍

},
Validators: []validator.Int64{
int64validator.Between(1024, 65535),
int64validator.NoneOf(2375, 2376, 7788, 7789, 7790, 7791, 7792, 7793, 7794, 7795, 7796, 7797, 7798, 7799, 8090, 51678, 51679, 51680),

Choose a reason for hiding this comment

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

(jk)

Suggested change
int64validator.NoneOf(2375, 2376, 7788, 7789, 7790, 7791, 7792, 7793, 7794, 7795, 7796, 7797, 7798, 7799, 8090, 51678, 51679, 51680),
int64validator.NoneOf(1337, 2375, 2376, 7788, 7789, 7790, 7791, 7792, 7793, 7794, 7795, 7796, 7797, 7798, 7799, 8090, 51678, 51679, 51680),

Copy link
Author

Choose a reason for hiding this comment

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

Is that a leetspeak reference?

@@ -215,6 +211,18 @@ func (r *resourceDBInstance) Schema(ctx context.Context, req resource.SchemaRequ
also use the InfluxDB CLI to create an operator token. These attributes will be
stored in a Secret created in AWS SecretManager in your account.`,
},
names.AttrPort: schema.Int64Attribute{
Optional: true,
Computed: true,

Choose a reason for hiding this comment

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

would it be redundant to add default here as well?

Suggested change
Computed: true,
Computed: true,
Default: int64(8086),

Copy link
Author

Choose a reason for hiding this comment

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

It would be redundant, the port defaults to 8086 anyways when creating an instance.

Copy link

@forestmvey forestmvey left a comment

Choose a reason for hiding this comment

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

One comment to address, then looks good.

Copy link

@forestmvey forestmvey left a comment

Choose a reason for hiding this comment

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

Looks good!

trevorbonas pushed a commit that referenced this pull request Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants