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

tests/resource/aws_iam_policy_attachment: Temporarily use expanded users references #7272

Merged
merged 1 commit into from
Jan 23, 2019

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Jan 23, 2019

This change is both backwards (0.11) and forwards (0.12) compatible to allow the configuration on both versions. Eventually the temporary workaround will be removed when we switch the provider test configurations to 0.12-only syntax.

Previous output from Terraform 0.12 acceptance testing:

--- FAIL: TestAccAWSIAMPolicyAttachment_paginatedEntities (2.16s)
    testing.go:568: Step 0 error: errors during plan:

        Error: Incorrect attribute value type

          on /opt/teamcity-agent/temp/buildTmp/tf-test032782444/main.tf line 26:
          (source code not available)

        Inappropriate value for attribute "users": element 0: string required.

Output from Terraform 0.12 acceptance testing:

--- PASS: TestAccAWSIAMPolicyAttachment_paginatedEntities (57.78s)

…ers references

This change is both backwards (0.11) and forwards (0.12) compatible to allow the configuration on both versions. Eventually the temporary workaround will be removed when we switch the provider test configurations to 0.12-only syntax.

Previous output from Terraform 0.12 acceptance testing:

```
--- FAIL: TestAccAWSIAMPolicyAttachment_paginatedEntities (2.16s)
    testing.go:568: Step 0 error: errors during plan:

        Error: Incorrect attribute value type

          on /opt/teamcity-agent/temp/buildTmp/tf-test032782444/main.tf line 26:
          (source code not available)

        Inappropriate value for attribute "users": element 0: string required.
```

Output from Terraform 0.12 acceptance testing:

```
--- PASS: TestAccAWSIAMPolicyAttachment_paginatedEntities (57.78s)
```
@bflad bflad added this to the v2.0.0 milestone Jan 23, 2019
@bflad bflad requested a review from a team January 23, 2019 18:45
@ghost ghost added size/M Managed by automation to categorize the size of a PR. service/iam Issues and PRs that pertain to the iam service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Jan 23, 2019
policy_arn = "${aws_iam_policy.policy.arn}"
users = [
"${aws_iam_user.user.*.name[0]}",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the temporary workaround you speak of the use of the splat operator? Reading the v0.12 preview I found https://www.hashicorp.com/blog/terraform-0-12-generalized-splat-operator#full-splat-operator so just want to confirm for learning purposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The temporary workaround here is explicitly declaring each element in the list as individual string references to maintain compatibility for Terraform 0.11 vendoring (in the likely case we will release more 1.X provider releases off master) and future compatibility for Terraform 0.12 vendoring. After some brief trial and error there does not seem to be an easy/friendly backwards and forwards compatible syntax that allows passing the list of strings directly even with various forms of using the splat operator. While there may actually be some decent syntax to accomplish this for longer-term real world usage (e.g. in public Terraform modules), these updates are only temporarily required in this acceptance testing, so opting for the first reasonable solution seems okay.

Once the Terraform AWS provider is out of its 2.0 beta period (to coincide with Terraform 0.12's beta period) and we are fairly confident we will not need to continue 1.X provider releases, we can update these acceptance test configurations to use the newer syntax and go back to a simplified configuration.

For operators upgrading to Terraform 0.12, they will theoretically be able to use the terraform 0.12upgrade command to automatically have their syntax simplified in this case from the existing:

users = ["${aws_iam_user.user.*.name}"]

To the newer:

users = aws_iam_user.user[*].name

Utilizing the full splat operator and bypassing errors seen trying to support both Terraform 0.11's configuration parser and Terraform 0.12's configuration parser.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for explaining.

Copy link
Contributor

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

This looks good. I left a question pertaining to the workaround you mentioned in the PR description.

@bflad bflad merged commit 0b8c5aa into master Jan 23, 2019
@bflad bflad deleted the td-aws_iam_policy_attachment-0.12-syntax branch January 23, 2019 20:39
@bflad bflad modified the milestones: v2.0.0, v2.1.0, v2.2.0 Feb 24, 2019
@bflad bflad modified the milestones: v2.2.0, 0.12-support Mar 14, 2019
@ghost
Copy link

ghost commented Mar 31, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
service/iam Issues and PRs that pertain to the iam service. size/M Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants