Add organisations data source #2530
Conversation
Ninir
added
the
enhancement
aws/data_source_aws_organizations.go
Outdated
| ForceNew: true, | ||
| }, | ||
| "accounts": { | ||
| Type: schema.TypeList, |
There was a problem hiding this comment.
Are we sure accounts are returned in order? if not, would be better to use a Set here
aws/data_source_aws_organizations.go
Outdated
| }) | ||
| if err != nil { | ||
| if orgerr, ok := err.(awserr.Error); ok && orgerr.Code() == "ParentNotFoundException" { | ||
| log.Printf("[WARN] Orgnization %s not found, removing from state", d.Id()) |
aws/data_source_aws_organizations.go
Outdated
| } | ||
|
|
||
| if len(accounts) < 1 { | ||
| return fmt.Errorf("Your query returned no results. Please change your account ID and try again") |
There was a problem hiding this comment.
what do you think about modifying this to Please check instead?
| Get AWS Organizations account ID's | ||
| --- | ||
|
|
||
| # aws\_organizations |
There was a problem hiding this comment.
Those antislashes are not needed anymore, it is safe to remove them
aws/data_source_aws_organizations.go
Outdated
| Read: dataSourceAwsOrganizationsAccountsRead, | ||
|
|
||
| Schema: map[string]*schema.Schema{ | ||
| "account_id": { |
There was a problem hiding this comment.
As a Terraform User, I would expect this data source to return Organisation information, not only members.
So first I would expect describe organisation attributes and then call the ListAccounts endpoint to get users.
It could even be a separated data source.
Thoughts?
Ninir
added
new-data-source
|
Hi @Ninir. I have made some changes, split the data sources into two, this should hopefully be clearer now. |
There was a problem hiding this comment.
Hey @DaveBlooman
This is much better, thank you a lot for your work and reactivity.
Would you be able to split this work in 2, so that we merge 1 data source in 1 PR?
This is getting into shape, will test it as soon as you can achieve that.
Thanks again! 🚀 ⭐️
| @@ -0,0 +1,91 @@ | |||
| package aws | |||
There was a problem hiding this comment.
Why not simply naming the file data_source_aws_organization.go? (and other ones below)
Isn't this the main object representing an Organization?
Ninir
added
the
waiting-response
|
@DaveBlooman will you have time to address PR feedback? No worries if not, we can finish this up for you. 👍 |
|
@bflad Probably wont have time this week, so go ahead :) |
bflad
removed
the
waiting-response
|
@dblooman I realize it's been awhile, but are you able to pick this back up. Since you last left off, a few new PRs duplicate and build on your work. If you're not able to, I'm happy to take it across the finish line for you. If I understand the feedback correctly (@Ninir correct me if I'm wrong), but the data sources (and file names) just need to be updated. For consistency, it seems like they should be updated to |
|
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
locked and limited conversation to collaborators
This adds the ability to get a list of account ID's from the organisations service.