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.

Sign up for GitHub

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

Jump to bottom

data-source/aws_network_interface: Add filter attribute #2851

Merged
merged 2 commits into from
Jan 29, 2018
Merged

data-source/aws_network_interface: Add filter attribute #2851

merged 2 commits into from
Jan 29, 2018

Conversation

Gufran
Copy link
Contributor

@Gufran Gufran commented Jan 3, 2018

No description provided.

@Gufran Gufran force-pushed the data-net-interface branch 2 times, most recently from 33651af to 74907d6 Compare January 3, 2018 19:01
@Ninir Ninir added the enhancement Requests to existing resources that expand the functionality or scope. label Jan 4, 2018
@bflad bflad added the service/ec2 Issues and PRs that pertain to the ec2 service. label Jan 11, 2018
@Gufran
Copy link
Contributor Author

Gufran commented Jan 29, 2018

@Ninir @bflad Any feedback on this change? Is it likely to go out with next release?

bflad
bflad reviewed Jan 29, 2018
View reviewed changes
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 @Gufran for this contribution! Check out the comments below and see if they make sense. Please let me know if you have any questions or when to check this again. 👍

aws/data_source_aws_network_interface.go Outdated
filters, filtersOk := d.GetOk("filter")
id, idOk := d.GetOk("id")
if idOk && filtersOk {
return fmt.Errorf("Only one of filter or id can be assigned")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a technical reason for this limitation?

  • If both id and filter are supplied and the filter successfully matches the resource corresponding to the ID, its okay and the data source will return the one result.
  • If both id and filter are supplied and the filter doesn't successfully match the resource corresponding to the ID, the data source will return the error.
  • Folks might want to create a module with this data source and toggle between both attributes. HCL doesn't support a nil value at the moment so this would prevent that use case.

aws/data_source_aws_network_interface.go Outdated
}

input := &ec2.DescribeNetworkInterfacesInput{}
if idOk {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably check to ensure ID actually has a value if idOk && id.(string) != "" {

aws/data_source_aws_network_interface_test.go
security_groups = ["${aws_security_group.test.id}"]
}

data "aws_network_interface" "test" {
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs depends_on = ["aws_network_interface.test"] otherwise it fails due to dependency ordering:

--- FAIL: TestAccDataSourceAwsNetworkInterface_filters (47.96s)
	testing.go:503: Step 0 error: Error applying: 1 error(s) occurred:

		* data.aws_network_interface.test: data.aws_network_interface.test: no matching network interface found

aws/data_source_aws_network_interface_test.go Outdated
`, rName)
}

func TestAccDataSourceAwsNetworkInterface_error_exclusive(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test and its configuration can be removed given above comment.

website/docs/d/network_interface.html.markdown Outdated
* `id` – (Optional) The identifier for the network interface.
* `filter` – (Optional) One or more name/value pairs to filter off of. There are several valid keys, for a full reference, check out [describe-network-interfaces](https://docs.aws.amazon.com/cli/latest/reference/ec2/describe-network-interfaces.html) in the AWS CLI reference.

~> **NOTE:** Either `id` or `filter` must be specified. Both `id` and `filter` cannot be specified together.
Copy link
Contributor

Choose a reason for hiding this comment

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

This note can be removed given above comment.

@bflad bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Jan 29, 2018
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Jan 29, 2018
@Gufran
Copy link
Contributor Author

Gufran commented Jan 29, 2018

@bflad I made the changes. and I'm sorry about the problem with tests, not sure if there is a way for me to run tests locally without racking up a bill on AWS.

@bflad bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Jan 29, 2018
@ghost ghost added size/M Managed by automation to categorize the size of a PR. and removed size/L Managed by automation to categorize the size of a PR. labels Jan 29, 2018
@bflad bflad added this to the v1.9.0 milestone Jan 29, 2018
@bflad
Copy link
Contributor

bflad commented Jan 29, 2018

Since you weren't able to run them, I made some minor adjustments to make the acceptance testing happy consistently (mainly setting id attribute Computed: true). Thanks so much for your work here! Merging! 🚀

make testacc TEST=./aws TESTARGS='-run=TestAccDataSourceAwsNetworkInterface'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccDataSourceAwsNetworkInterface -timeout 120m
=== RUN   TestAccDataSourceAwsNetworkInterface_basic
--- PASS: TestAccDataSourceAwsNetworkInterface_basic (33.42s)
=== RUN   TestAccDataSourceAwsNetworkInterface_filters
--- PASS: TestAccDataSourceAwsNetworkInterface_filters (31.90s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	65.364s

@bflad bflad changed the title Add filters in network interface data source data-source/aws_network_interface: Add filter attribute Jan 29, 2018
@bflad bflad merged commit 1a7da61 into hashicorp:master Jan 29, 2018
bflad added a commit that referenced this pull request Jan 29, 2018
@bflad
Update CHANGELOG for #2851
01ca9ae
@Gufran Gufran deleted the data-net-interface branch January 30, 2018 04:05
@bflad
Copy link
Contributor

bflad commented Feb 9, 2018

This has been released in terraform-provider-aws version 1.9.0. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

@ghost
Copy link

ghost commented Apr 8, 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 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bflad bflad bflad left review comments

Assignees
No one assigned
Labels
enhancement Requests to existing resources that expand the functionality or scope. service/ec2 Issues and PRs that pertain to the ec2 service. size/M Managed by automation to categorize the size of a PR.
Projects
None yet
Milestone
v1.9.0
Development

Successfully merging this pull request may close these issues.
3 participants
@Gufran @bflad @Ninir