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

data_source_aws_security_group: add description #1943

Merged
merged 1 commit into from
Oct 18, 2017

Conversation

darkowlzz
Copy link
Contributor

@darkowlzz darkowlzz commented Oct 17, 2017

From #1940 I came across https://www.terraform.io/docs/providers/aws/d/security_group.html#description where description is an attribute reference. But in real, description isn't available. This change adds description to data_source_aws_security_group.

This doesn't f-ixes #1940, which talks about description for the SG rules, which I see already exist 🤷‍♀️.

Copy link
Contributor

@Ninir Ninir left a comment

Choose a reason for hiding this comment

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

Hey @darkowlzz

This looks good! 👍

Just left a nitpick. Also, can you ad the attribute in the documentation? would help people knows this exists.

Thanks!


"description": {
Type: schema.TypeString,
Optional: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional is used when the user can pass a value for this attribute. Since we are only retrieving the description (and we don't need it to retrieve the Security Group info), this is not needed here and can be removed

@Ninir Ninir added the enhancement Requests to existing resources that expand the functionality or scope. label Oct 18, 2017
@darkowlzz
Copy link
Contributor Author

darkowlzz commented Oct 18, 2017

@Ninir thanks for the review.

Removed Optional and combined with the same commit.

About adding the attribute to documentation, isn't it already there in the docs? https://github.com/terraform-providers/terraform-provider-aws/blob/master/website/docs/d/security_group.html.markdown#attributes-reference

Should I add it to some other docs too?

Copy link
Contributor

@Ninir Ninir left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot @darkowlzz! 👍

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

@ghost
Copy link

ghost commented Apr 10, 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 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Requests to existing resources that expand the functionality or scope.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants