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

provider/aws: Add aws_account_id data source #8206

Merged
merged 1 commit into from
Aug 16, 2016
Merged

Conversation

jen20
Copy link
Contributor

@jen20 jen20 commented Aug 15, 2016

This data source provides access during configuration to the ID of the AWS account for the connection to AWS. It is primarily useful for interpolating into policy documents, for example when creating the policy for an ELB or ALB access log bucket.

This will need revisiting and further testing once the work for AssumeRole is integrated.

@apparentlymart
Copy link
Contributor

This seems to be an implementation for #4390. (Just noting for the benefit of those following the other one.)

@jen20
Copy link
Contributor Author

jen20 commented Aug 15, 2016

@apparentlymart I prefer your terminology as it allows for expansion later. I'll update this PR.

@radeksimko
Copy link
Member

Re terminology: keep in mind the account ID may not always come from STS at this point, although we could probably ditch all other methods now when we have STS:GetCallerIdentity. It seems to be very reliable way of getting account ID. I understand your point about expansion though.

Although it may be obvious, would you mind adding a warning/notice to docs that account ID may be unavailable under certain circumstances?

https://github.com/hashicorp/terraform/blob/master/website/source/docs/providers/aws/index.html.markdown => if skip_requesting_account_id == true

Also the data source should IMO be returning error in such case ^.

@jen20
Copy link
Contributor Author

jen20 commented Aug 15, 2016

@radeksimko, @apparentlymart - Thanks for the feedback here. I've adjusted the terminology to match the discussion in #4390 and added the error condition suggested by @radeksimko!

log.Printf("[DEBUG] Reading Caller Identity.")
d.SetId(time.Now().UTC().String())

if client.accountid != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks inverted... don't we want to produce this error if the account id is empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hah yes. Apparently last minute alterations are what code review is intended to catch ;-)

@stack72
Copy link
Contributor

stack72 commented Aug 16, 2016

LGTM! Nice addition :)

Read: dataSourceAwsCallerIdentityRead,

Schema: map[string]*schema.Schema{
"account_id": {
Copy link
Member

Choose a reason for hiding this comment

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

This field is not actually set in Read, is it?

This data source provides access during configuration to the ID of the
AWS account for the connection to AWS. It is primarily useful for
interpolating into policy documents, for example when creating the
policy for an ELB or ALB access log bucket.

This will need revisiting and further testing once the work for
AssumeRole is integrated.
@jen20
Copy link
Contributor Author

jen20 commented Aug 16, 2016

@radeksimko fixed up now.

› envchain aws.terraform make testacc TEST=./builtin/providers/aws TESTARGS=" -run TestAccAWSCallerIdentity_basic"
==> Checking that code complies with gofmt requirements...
/Users/James/Code/go/bin/stringer
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/08/16 11:30:19 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run TestAccAWSCallerIdentity_basic -timeout 120m
=== RUN   TestAccAWSCallerIdentity_basic
--- PASS: TestAccAWSCallerIdentity_basic (11.55s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    11.575s

@jen20 jen20 merged commit 90bdaef into master Aug 16, 2016
@jen20 jen20 deleted the f-aws-account-id branch August 16, 2016 10:33
@ghost
Copy link

ghost commented Apr 23, 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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants