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

Add an aws_resource_group resource for managing AWS resource groups #6217

Merged
merged 17 commits into from
Jan 10, 2019

Conversation

joestump
Copy link
Contributor

@joestump joestump commented Oct 19, 2018

What is the purpose of this PR?

Introduces an aws_resource_group resource.

Example HCL

resource "aws_resource_group" "test" {
  name        = "%s"
  description = "Hello World"

  resource_query {
    query = <<JSON
{
  "ResourceTypeFilters": [
    "AWS::EC2::Instance"
  ],
  "TagFilters": [
    {
      "Key": "Stage",
      "Values": ["Test"]
    }
  ]
}
JSON
  }
}

Test outputs

make testacc TESTARGS='-run=TestAccAWSResourceGroup'
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAWSResourceGroup -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSResourceGroup_importBasic
=== PAUSE TestAccAWSResourceGroup_importBasic
=== RUN   TestAccAWSResourceGroup_basic
=== PAUSE TestAccAWSResourceGroup_basic
=== CONT  TestAccAWSResourceGroup_importBasic
=== CONT  TestAccAWSResourceGroup_basic
--- PASS: TestAccAWSResourceGroup_basic (17.90s)
--- PASS: TestAccAWSResourceGroup_importBasic (19.07s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	20.443s

TODO

Joe Stump and others added 7 commits October 17, 2018 21:30
…cegroups@v1.15.57

Updates:
* `govendor fetch github.com/aws/aws-sdk-go/...@v1.15.57`
* `govendor fetch github.com/aws/aws-sdk-go/service/resourcegroups@v1.15.57`
…s/terraform-provider-aws into jstump-resource-groups-sdk
@ghost ghost added dependencies Used to indicate dependency changes. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. size/XXL Managed by automation to categorize the size of a PR. labels Oct 19, 2018
@joestump joestump changed the title Add an aws_resource_group resource for managing AWS resource groups WIP: Add an aws_resource_group resource for managing AWS resource groups Oct 19, 2018
@bflad bflad added new-resource Introduces a new resource. service/resourcegroups Issues and PRs that pertain to the resourcegroups service. labels Oct 19, 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 for submitting this @joestump! Initial feedback left below. Please reach out with any questions.

aws/resource_aws_resource_group.go Outdated Show resolved Hide resolved
aws/resource_aws_resource_group.go Outdated Show resolved Hide resolved
aws/resource_aws_resource_group.go Outdated Show resolved Hide resolved
aws/resource_aws_resource_group.go Outdated Show resolved Hide resolved
aws/resource_aws_resource_group.go Outdated Show resolved Hide resolved
aws/resource_aws_resource_group.go Outdated Show resolved Hide resolved
aws/resource_aws_resource_group_test.go Outdated Show resolved Hide resolved
aws/resource_aws_resource_group_test.go Outdated Show resolved Hide resolved
aws/resource_aws_resource_group_test.go Outdated Show resolved Hide resolved
aws/provider.go Outdated Show resolved Hide resolved
@bflad
Copy link
Contributor

bflad commented Oct 19, 2018

Oh! I would also recommend naming the resource (and updating file names) to aws_resourcegroups_group -- it looks uglier but we have been trying to stick with aws_SERVICE_THING where possible to prevent namespace collisions in the future. See also: aws_organizations_organization 😅

@ghost ghost added the documentation Introduces or discusses updates to documentation. label Oct 19, 2018
@joestump
Copy link
Contributor Author

@bflad I think comments have been addressed. 👍

@joestump joestump changed the title WIP: Add an aws_resource_group resource for managing AWS resource groups Add an aws_resource_group resource for managing AWS resource groups Oct 19, 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.

Two more little things and this should be good to go, thanks so much!

aws/resource_aws_resourcegroups_group_test.go Show resolved Hide resolved

## Import

IAM Users can be imported using the `name`, e.g.
Copy link
Contributor

Choose a reason for hiding this comment

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

Copypasta here and the example command 🍝 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha! Literally had that committed about 30 minutes ago and forgot to push. 🤦‍♂️

@joestump
Copy link
Contributor Author

joestump commented Oct 20, 2018

@bflad I've got an interesting issue with this resource. Turns out you can set tags when you call CreateGroup, but GetGroup and ListGroups don't return the groups tags. This is causing the import test to fail if tags are present as it expects tags to be instate, but I can't pull them from the API.

My proposal would be to:

  • Remove import functionality for now.
  • Mark tags as ForceNew.

The other option would be to remove this attribute entirely I guess? I've sent an email to our AWS TAM asking they make the tags retrievable.

@bflad
Copy link
Contributor

bflad commented Oct 30, 2018

For reading tags, I believe GetTags is what you are looking for, passing in the ARN of the group. For updating tags, I believe Tag and Untag 😄

Otherwise feel free to remove the group tagging bits out for now and we can get in separately.

@bflad bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Oct 31, 2018
@bflad
Copy link
Contributor

bflad commented Oct 31, 2018

FYI aws/aws-sdk-go/service/resourcegroups is now vendored in via: #6308

@joestump
Copy link
Contributor Author

I'll give GetTags a try with a resource group's ARN. For some reason it hadn't dawned on me that might work.

@joestump
Copy link
Contributor Author

@bflad okay, so looking over these endpoints I've got a new proposal for this. I think we should remove tags from this resource and create two additional resources:

  • aws_resourcegroups_tags - Data resource that takes an ARN and returns a map of tags for ARN.
  • aws_resourcegroups_tags - Regular resource that takes an ARN and tags it.

The nice thing about this is that it works for any ARN.

Thoughts?

@bflad
Copy link
Contributor

bflad commented Oct 31, 2018

@joestump are you positive that those API calls are intended for outside Resource Groups themselves?

e.g. https://docs.aws.amazon.com/ARG/latest/APIReference/API_GetTags.html

Arn

The ARN of the resource for which you want a list of tags. The resource must exist within the account you are using.

Pattern: arn:aws:resource-groups:[a-z]{2}-[a-z]+-\d{1}:[0-9]{12}:group/[a-zA-Z0-9_.-]{1,128}

https://docs.aws.amazon.com/ARG/latest/APIReference/API_Tag.html

Arn

The ARN of the resource to which to add tags.

Pattern: arn:aws:resource-groups:[a-z]{2}-[a-z]+-\d{1}:[0-9]{12}:group/[a-zA-Z0-9_.-]{1,128}

Either way, the group itself should support tagging via the above.

@bflad
Copy link
Contributor

bflad commented Oct 31, 2018

The API for tagging resources via resource groups is separate and can be found here:

@joestump
Copy link
Contributor Author

@bflad BRB learning to read good.

@bflad
Copy link
Contributor

bflad commented Nov 13, 2018

Hey @joestump 👋 Will you have time to circle back and finish this up? If not, no problem.

@joestump
Copy link
Contributor Author

@bflad I should be able to. Sorry for the delay, the usual things have kept me from revisiting lately.

@ghost ghost added size/XL Managed by automation to categorize the size of a PR. provider Pertains to the provider itself, rather than any interaction with AWS. and removed size/XXL Managed by automation to categorize the size of a PR. labels Nov 15, 2018
@bflad bflad removed the dependencies Used to indicate dependency changes. label Nov 21, 2018
@bflad bflad added this to the v1.55.0 milestone Jan 10, 2019
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.

Hey @joestump 👋 Since its been two months and we haven't heard anything, I went ahead and made the minor adjustments (8375b2f) so we could ship at least the initial aws_resourcegroups_group resource without tagging support. I'll create a followup issue for adding that in that you or anyone else can pick. Thanks for your work here! 🚀

Output from acceptance testing:

--- PASS: TestAccAWSResourceGroup_basic (20.04s)

@bflad bflad merged commit d6748b7 into hashicorp:master Jan 10, 2019
bflad added a commit that referenced this pull request Jan 10, 2019
@bflad
Copy link
Contributor

bflad commented Jan 10, 2019

This has been released in version 1.55.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

@ghost ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Jan 10, 2019
@joestump
Copy link
Contributor Author

@bflad sorry for going MIA. Got slammed with a big deadline at work. I've got tagging with proper tests and whatnot locally. I'll try and clean it up for PR soon!

@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
documentation Introduces or discusses updates to documentation. new-resource Introduces a new resource. provider Pertains to the provider itself, rather than any interaction with AWS. service/resourcegroups Issues and PRs that pertain to the resourcegroups service. size/XL 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