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

Fix bad advice on using ".name" property of Role while ignoring path #6285

Merged

Conversation

rehevkor5
Copy link
Contributor

Changes proposed in this pull request:

  • When creating an IAM Role with a non-default path (anything other than
    "/"), it is not sufficient to use only the "name" property of the Role
    when referring to it from the Glue Crawler resource (and probably
    others as well). Therefore, in the examples I have replaced usage of
    "name" with usage of "arn", and I updated the documentation on the
    "role" argument of the aws_glue_crawler resource to clarify the meaning
    of "The IAM role" which was not well-defined.
  • Relevant Terraform version: This update can be deployed to the site
    immediately, it is not specific to an upcoming version of Terraform.

Output from acceptance testing:

Not performed.

- When creating an IAM Role with a non-default path (anything other than
"/"), it is not sufficient to use only the "name" property of the Role
when referring to it from the Glue Crawler resource (and probably
others as well). Therefore, in the examples I have replaced usage of
"name" with usage of "arn", and I updated the documentation on the
"role" argument of the aws_glue_crawler resource to clarify the meaning
of "The IAM role" which was not well-defined.
- Relevant Terraform version: This update can be deployed to the site
immediately, it is not specific to an upcoming version of Terraform.
@ghost ghost added size/XS Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. service/glue Issues and PRs that pertain to the glue service. labels Oct 26, 2018
@rehevkor5
Copy link
Contributor Author

Related: #6287

@bflad
Copy link
Contributor

bflad commented Oct 28, 2018

So while this change is for the better, it turns out it does not work immediately out of the box. 😅 It looks like even if you pass in the IAM role via ARN, the Glue API will return it with just the name. I found this by adjusting the role references in aws/resource_glue_crawler_test.go and running the acceptance testing via: make testacc TEST=./aws TESTARGS='-run=TestAccAWSGlueCrawler_'

(other test results similar)

--- FAIL: TestAccAWSGlueCrawler_DynamodbTarget (18.09s)
    testing.go:538: Step 0 error: After applying this step, the plan was not empty:

        DIFF:

        UPDATE: aws_glue_crawler.test
          role: "tf-acc-test-5817417459117994664" => "arn:aws:iam::123456789012:role/tf-acc-test-5817417459117994664"

I will file this as a separate bug as we'll need to suppress that difference in both the case of a role with and without a path. I'm not sure if we should merge in this documentation update immediately though since at least a role name without path should work in some cases for new practitioners while ARN will not work in any case unless ignore_changes is used.

@bflad
Copy link
Contributor

bflad commented Oct 28, 2018

Bug report submitted: #6292

I'll try to get in a fix for that right now.

@bflad
Copy link
Contributor

bflad commented Oct 28, 2018

This pull request can be merged after #6293 👍

@rehevkor5
Copy link
Contributor Author

rehevkor5 commented Oct 29, 2018

@bflad Sorry about that. Thanks for checking & helping. I guess there were some aspects of this I missed due to not running the acceptance tests. I should mention though that using ARN worked ok on my Terraform stack. Not sure what circumstances would cause problems... maybe updating an existing stack?

@bflad
Copy link
Contributor

bflad commented Oct 30, 2018

@rehevkor5 are your IAM roles in a different AWS account? Maybe the API takes that into consideration, where it attempts to simplify its response when the role is in the same account as the Glue crawler. 😄 Regardless though, #6293 is merged which covers suppressing the Name -> ARN difference for whatever situation our testing account against us-west-2 was running into so this is good to go now!

@bflad bflad added this to the v1.42.0 milestone Oct 30, 2018
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Thanks so much, @rehevkor5! 🚀

@bflad bflad merged commit d902b9b into hashicorp:master Oct 30, 2018
@ghost
Copy link

ghost commented Apr 2, 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 Apr 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. service/glue Issues and PRs that pertain to the glue service. size/XS Managed by automation to categorize the size of a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants