add quicksight data set resource

[lacernetic]

Community Note

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

Relates: #10990

Output from acceptance testing:

TESTS INCOMPLETE: SEE BELOW FOR MORE DETAILS

NOTES FOR REVIEWER:

• Build is failing on the pipeline and the error involves an import. It builds fine locally so I'm not sure what the problem is.
• Included a virtual attribute to allow the user to choose the name of the physical_table but it's causing a state failure. Once this gets fixed, I want to implement this into field_folders and logical_table_map.
• Every test that has a comment above the test function is failing for the reason given in the comment. We may need to circle up for a better explanation on why things are failing and what I've tried to fix. Those without comments are passing.
• Cannot confirm data set has been created due to a lack of output status (line 848 on data_set.go).
• When I include output_columns I run into an error
• I basically copied the permisisons and tags attribute from quicksight_data_source so I'm not confident if I did that correctly.
• I'm not confident in the import part of the documentation. Don't really know what it does/how to test.

[tb102122]

@lacernetic do you have an plan date when this will bw completed?

[lacernetic]

@lacernetic do you have an plan date when this will bw completed?

Current plan is to have this merged in by early February!

[zhelding]

Ideally, the default is being created consistently with the same value, in which case we should specify the outputted value as a Default for this attribute. (terraform-plugin-sdk reference)

Otherwise, you are correct that we'll need to resort to Computed: true to prevent the non-empty plan in acceptance testing.

[zhelding]

Suggested change

ExactlyOneOf: []string{"custom_sql", "relational_table", "s3_source"},

Trying to implement this myself: I have learned that both ConflictsWith and ExactlyOneOf are only supported for schema of TypeList with MaxItems: 1!

I will have to ask around, but I believe this means that we can't do any provider-level check to ensure that only one attribute of a list element is present within that element. As such, we can exclude ExactlyOneOf from physical_table_map here.

[zhelding]

Suggested change

	"physical_table_map_id": {
	"physical_table_id": {

The ID here is referring to the unique key identifying each individual PhysicalTable -- so I think it makes more sense to remove "map" here.

Additionally, I believe that it is this ID that is being referenced in logical_table_map.0.source as physical_table_id -- so it would be clean for those to match.

[zhelding]

Suggested change

	"field_folders_id": {
	"field_folder_id": {

Similarly to the instance of using physical_table_map_id above, the ID here refers to the unique key for an individual field folder.

[zhelding]

"schema" should have a ValidateFunc to ensure it is no longer than 64 characters (API reference)

[zhelding]

Suggested change

	columnLevelPermissionRule.ColumnNames = flex.ExpandStringList(v)
	columnLevelPermissionRule.Principals = flex.ExpandStringList(v)

[zhelding]

Hi @lacernetic! I've gone ahead and made some more comments and suggestions here.

I think the main blocker to testing / progress at this point is the error you identified in ExactlyOneOf due to attributes being referenced incorrectly; luckily, this is an easy fix of specifying the exact block in which an attribute is nested (link to discussion).

However, I have noted during my own experimentation that ExactlyOneOf -- as well as ConflictsWith -- turns out to only be supported for schema of TypeSet with MaxItems: 1 (link to discussion).

Therefore, ExactlyOneOf would be usable with logical_table_map as written -- but as noted in my previous review: logical_table_map ought to be adapted into a full map in the same manner as physical_table_map and field_folders. Specifically, logical_table_map should have its MaxItems increased, and a logical_table_id field should be specified.

As a result, I do not think we will be able to make use of either ExactlyOneOf or ConflictsWith in any of the locations where it might make sense -- i.e. where AWS requires one and only one particular attribute of a set to be specified. I will have to ask around to see if there are any alternative methods we might use here -- but otherwise we might simply not be able to validate this on the provider-level.

For now certainly: we should abandoned the ExactlyOneOf statements, adapt logical_table_map, and try to get passing acceptance tests.

Thanks again for your contribution and work on this!

[lacernetic]

@zhelding I just uploaded my changes for physical_table_map as a set. However when I run the code I get an error:

▶ make testacc TESTS=TestAccQuickSightDataSet_basic PKG=quicksight
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/quicksight/... -v -count 1 -parallel 20 -run='TestAccQuickSightDataSet_basic' -timeout 180m
=== RUN   TestAccQuickSightDataSet_basic
=== PAUSE TestAccQuickSightDataSet_basic
=== CONT  TestAccQuickSightDataSet_basic
    data_set_test.go:25: Step 1/2 error: After applying this test step, the plan was not empty.
        stdout:


        Terraform used the selected providers to generate the following execution
        plan. Resource actions are indicated with the following symbols:
          ~ update in-place

        Terraform will perform the following actions:

          # aws_quicksight_data_set.dset will be updated in-place
          ~ resource "aws_quicksight_data_set" "dset" {
                id             = "264522182925/tf-acc-test-4064235328383894385"
                name           = "tf-acc-test-4480227932746657673"
                # (5 unchanged attributes hidden)



              - physical_table_map {
                  - physical_table_map_id = "s3PhysicalTable" -> null

                  - s3_source {
                      - data_source_arn = "arn:aws:quicksight:us-west-2:264522182925:datasource/tf-acc-test-4064235328383894385" -> null

                      - input_columns {
                          - name = "ColumnId-1" -> null
                          - type = "STRING" -> null
                        }

                      - upload_settings {
                          - contains_header = true -> null
                          - format          = "JSON" -> null
                          - start_from_row  = 0 -> null
                          - text_qualifier  = "DOUBLE_QUOTE" -> null
                        }
                    }
                }
              + physical_table_map {
                  + physical_table_map_id = "tf-acc-test-4064235328383894385"

                  + s3_source {
                      + data_source_arn = "arn:aws:quicksight:us-west-2:264522182925:datasource/tf-acc-test-4064235328383894385"

                      + input_columns {
                          + name = "ColumnId-1"
                          + type = "STRING"
                        }

                      + upload_settings {
                          + contains_header = (known after apply)
                          + delimiter       = (known after apply)
                          + format          = (known after apply)
                          + start_from_row  = (known after apply)
                          + text_qualifier  = (known after apply)
                        }
                    }
                }
                # (2 unchanged blocks hidden)
            }

        Plan: 0 to add, 1 to change, 0 to destroy.
--- FAIL: TestAccQuickSightDataSet_basic (46.62s)
FAIL
FAIL	github.com/hashicorp/terraform-provider-aws/internal/service/quicksight	50.449s
FAIL
make: *** [testacc] Error 1

I did some checking around the only difference between the original table and the flatten version is that one of them has default upload_settings (set by AWS) while the other does not. Upload settings is marked as computed so I'm not sure why these are being counted in the test. Any thoughts?

[zhelding]

Hi @lacernetic! I'm hoping to dig into this come Monday -- but for now: I think the test output you posted is showing drift detected in physical_table_map_id? Specifically: it seems as though when terraform plan is called, the value in state is somehow s3PhysicalTable as opposed to the generated testing value.

[lacernetic]

Hi @lacernetic! I'm hoping to dig into this come Monday -- but for now: I think the test output you posted is showing drift detected in physical_table_map_id? Specifically: it seems as though when terraform plan is called, the value in state is somehow s3PhysicalTable as opposed to the generated testing value.

From my experience this "change in plan" usually happens when the flatten function in read expects something different from what is being set in create. However I checked both what things and both are outputting the same value given from the test. Definitely more than stuck on this one :(

[zhelding]

Hi @lacernetic.

I consulted with @YakDriver on this, and unfortunately we concluded that the AWS API is simply not behaving as expected here.

The input of the CreateDataSet operation (including PhysicalTableMap) is definitely being correctly specified and passed without error, and the docs clearly list the PhysicalTable string keys as writable inputs; however, DescribeDataSet is consistently returning a different value.

Either the API ought to be accepting the PhysicalTable string keys and returning them as inputted in the Describe operation -- or the docs should tell us what output to expect from DescribeDataSet.

Regrettably, this seems to be a blocking issue for this resource: the LogicalTableMap relies on these string keys to reference PhysicalTable sources -- so the value of the string keys must be known at plan time.

We have gone ahead and submitted a support ticket to AWS re: this unexpected API behavior. Hopefully, AWS can either patch their API or update their documentation -- and then we can resume work on this resource.

Thanks so much for your continued patience and contributions. I recognize it's a pain to have work stall like this, so we greatly appreciate you sticking with the process!

[lacernetic]

@zhelding Is there an estimated time on when AWS will get back to us on this ticket?

[zhelding]

Hi @lacernetic. Sorry for the delayed reply here.

We have been coordinating with our Solutions Architect and AWS Support on our end, and they have acknowledged the issue; however, they are unable to provide a timeline as to when it might be resolved.

I will continued to chase that up as time passes. Thank you very much for your patience.

[lacernetic]

Awesome thanks for the update!! I expected for this to be a long process so no worries here!

[lacernetic]

Hi @zhelding !! Are there any updates from AWS on the timeline of fixing this issue?

[jar-b]

Edit: The potential options have been expanded and re-ordered.

---

Quick research update. I'm unable to push to the remote for this PR, so I've started a working branch with updates (see the diff here f_quicksight_data_set_resource...f-aws_quicksight_data_set).

After bringing the changes up to date with main, many tests still have failures due to persistent diffs in the physical_table_map[*].s3_source.upload_settings attribute. The root cause is that the DescribeDataSet API returns a populated object of upload settings, even when omitted on create or update. When this value is present in state, but not the configuration, subsequent applies will always attempt to remove it.

Handling these optional/computed attributes properly, especially when deeply nested inside a required attribute, is a difficult design to get right. The following list describes some potential paths forward:

• Set upload_settings in state only when configured.
  • Read operations will skip writing this attribute to state, avoiding inconsistent final plans when default values are added.
  • Con: There will be no drift detection on this attribute.
• Make physical_table_map.*.s3_source.0.upload_settings optional and set default values.
  • The UploadSettings Documentation does not indicate any standard default values. I've opened a support case with AWS to determine if we can safely assume default values when settings are omitted on create or update.
  • (Potential) Con: Defaults may vary across use case/source types. Setting a default value (rather that omitting and allowing AWS to set the defaults) could cause unintended consequences.
• Migrate to Plugin-Framework.
  • This library exposes granular plan modifiers which can handle these cases more gracefully (see https://github.com/hashicorp/terraform-provider-aws/compare/f-aws_quicksight_data_set-framework for a minimal working implementation with plan modifiers on the upload_settings attribute).
  • Con: Requires porting the remainder of the resource to the branch above.
• Switch physical_table_map to a list type.
  • A CustomizeDiff function may be necessary since the underlying AWS map type likely cannot guarantee order.
  • Con: Significant additional logic required to implement CustomizeDiff.
• Make s3_source.0.upload_settings required.
  • This will force practitioners to always provide this object (even though AWS does not require it), but avoids a deeply nested computed object.
  • Con: Requires extra configuration, does not align with the AWS API definition.

Once we have more information from the AWS support case I'll update here on the direction we intend to pursue.

[jar-b]

After thinking on this more, I suggest we proceed with the first option above (skip writing upload_settings to state on read). This option is the easiest to adjust later, and will allow us to move forward with testing.

If the AWS response from the support case indicates default values are always consistent when upload_settings is omitted, we can then set the appropriate defaults and regain drift detection. If not, we can weight the absence of drift detection against the effort to port completely to Plugin-Framework. If the value of drift detection is substantial enough we can change course accordingly.

[jar-b]

This work has been completed and merged with #30349.

To round out the discussion on physical_table_map.s3_source.upload_settings - when omitted, the default values are determined by the manifest object in the source S3 bucket. This means there is no standard default we could use across all cases. We've opted to make this block required to avoid complications resolving optional/computed diffs nested within a set. Practitioners can either set an empty block (upload_settings {}) to inherit the computed values, or pass the same values configured in the S3 manifest object.

[github-actions]

This functionality has been released in v4.62.0 of the Terraform AWS Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.