Normalized the IAM role data source

[Ninir]

This normalizes the IAM Role data source to match the resource:

• Arguments & attributes are now named the same as in the resource
• The data source acceptance test has been fixed to include a variabilized part
• It is now using the read method from the resource, which handles more cases

Fixes #1129

Tests

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSDataSourceIAMRole_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSDataSourceIAMRole_basic -timeout 120m
=== RUN   TestAccAWSDataSourceIAMRole_basic
--- PASS: TestAccAWSDataSourceIAMRole_basic (35.26s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	35.291s

[radeksimko]

This looks otherwise sensible, just left you a comment regarding deprecation policy - we prefer not to break things for people without prior warning.

[radeksimko]

I think we should deprecate things in the first stage and then eventually phase them out after a few releases, in a separate PR.

[Ninir]

Thanks for the review @radeksimko . Just made the related changes!

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSDataSourceIAMRole_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSDataSourceIAMRole_basic -timeout 120m
=== RUN   TestAccAWSDataSourceIAMRole_basic
--- PASS: TestAccAWSDataSourceIAMRole_basic (20.92s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	20.955s

[radeksimko]

I left you just one really low prio question - this LGTM.

[radeksimko]

Nice work with meaningful variable names here 👍

[radeksimko]

What's the benefit of errwrap here, as we're not wrapping any error? Can't it be either fmt.Errorf() (with name being argument) or errors.New()?

[Ninir]

When looking at Wrapf, it states:

Wrapf wraps an error with a formatting message. This is similar to using fmt.Errorf to wrap an error. If you're using fmt.Errorf to wrap errors, you should replace it with this.

Find it better to use it than fmt.Errorf, but errors.New could still do it. Do you prefer one or the other?

[radeksimko]

I don't think this is about one or the other - both have valid use cases.

As you rightly quote from errwrap "... to wrap errors ...". 😉 We don't have a real error (as in nothing matching error interface) as we're literally constructing new string-based one here. errwrap is meant to be used for cases where you get a typed error (e.g. from AWS SDK) and you want to add some extra context without hiding the original error behind.

fmt.Errorf("Extra context: %s", origErr) comes back as &errors.errorString{s:"Extra context: stringified original error"}. You have no way to reliably figure out what the original error was, except by doing string match of the message. Errwrap provides way to do this: https://github.com/hashicorp/errwrap#custom-types

We'd rarely use errwrap when returning error from CRUD, because there's unlikely going to be anything relying on the error type in the rest of the stack. We only present it to the user as text.

Let me know if that explanation makes sense.

[Ninir]

Makes total sense, thanks for the precision there. Should be all ok.

[radeksimko]

LGTM, merge at will. 🙂

[ghost]

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!