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

New Resource: aws_iot_thing_type #3302

Merged
merged 1 commit into from
Feb 20, 2018
Merged

New Resource: aws_iot_thing_type #3302

merged 1 commit into from
Feb 20, 2018

Conversation

Ninir
Copy link
Contributor

@Ninir Ninir commented Feb 8, 2018 •
edited
Loading

Description

This adds a new resource for IoT called aws_iot_thing_type.

Related issues

Tests

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSIotThingTypeTest_'          
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSIotThingTypeTest_ -timeout 120m
=== RUN   TestAccAWSIotThingTypeTest_importBasic
--- PASS: TestAccAWSIotThingTypeTest_importBasic (327.51s)
=== RUN   TestAccAWSIotThingTypeTest_basic
--- PASS: TestAccAWSIotThingTypeTest_basic (324.77s)
=== RUN   TestAccAWSIotThingTypeTest_full
--- PASS: TestAccAWSIotThingTypeTest_full (340.52s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	992.847s

@Ninir Ninir added enhancement Requests to existing resources that expand the functionality or scope. new-resource Introduces a new resource. service/iot Issues and PRs that pertain to the iot service. labels Feb 8, 2018
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 8, 2018
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 8, 2018
@Ninir Ninir added this to the v1.9.0 milestone Feb 8, 2018
@Ninir Ninir requested a review from bflad February 8, 2018 23:28
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 8, 2018
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 8, 2018
@Ninir Ninir requested a review from radeksimko February 9, 2018 08:01
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 9, 2018
radeksimko
radeksimko reviewed Feb 9, 2018
View reviewed changes
aws/resource_aws_iot_thing_type.go Outdated
d.Set("deprecated", out.ThingTypeMetadata.Deprecated)
}

d.Set("arn", *out.ThingTypeArn)
Copy link
Member

Choose a reason for hiding this comment

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

Can you defer the dereferencing to Set() and prevent potential crash here?

aws/resource_aws_iot_thing_type.go
func resourceAwsIotThingTypeUpdate(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*AWSClient).iotconn

if d.HasChange("deprecated") {
Copy link
Member

Choose a reason for hiding this comment

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

Unless I'm mistaken this will always be triggered for new resources during creation, even though it's IMO not necessary as we can assume new resource doesn't need "undeprecation" (no-op API call)?

Copy link
Member

Choose a reason for hiding this comment

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

Theoretically we could just modify the condition here to also check for d.IsNewResource() and the real value of deprecated.

aws/resource_aws_iot_thing_type.go Outdated
return resource.NonRetryableError(err)
}

d.SetId("")
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: I know we have that in many resources still, but it's redundant. The ID is emptied automatically in the core.

See https://github.com/hashicorp/terraform/blob/096847c9f774bfb946de7413260b30f9f6041241/helper/schema/resource.go#L197-L206

aws/import_aws_iot_thing_type_test.go Outdated
"github.com/hashicorp/terraform/helper/resource"
)

func TestAccAWSIotThingTypeTest_importbasic(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: importBasic #ocd

aws/resource_aws_iot_thing_type.go Outdated
Type: schema.TypeString,
ValidateFunc: validateIotThingTypeSearchableAttribute,
},
},
Copy link
Member

Choose a reason for hiding this comment

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

Is there any particular reason we should be drifting away from the API here? i.e. not nest description and searchable_attributes under properties?

Obviously we don't know how is the API going to evolve. It's a guessing game at this point, but it has proven to me in the past that it's better to stick to the API. 😉

@cemo
Copy link

cemo commented Feb 12, 2018

@radeksimko this issue is still targeted to v1.9.0, It might be better to set v1.10.0 ?

@bflad bflad modified the milestones: v1.9.0, v1.10.0 Feb 12, 2018
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 19, 2018
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 19, 2018
@Ninir
Copy link
Contributor Author

Ninir commented Feb 19, 2018

@radeksimko Should be all ok now :)

radeksimko
radeksimko approved these changes Feb 20, 2018
View reviewed changes
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

Thanks, just left you some nitpicky comments, but no blockers.

LGTM.

website/docs/r/iot_thing_type.html.markdown Outdated

In addition to the arguments above, the following attributes are exported:

* `arn` - The ARN of the created AWS IoT thing_type
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: thing_type -> Thing Type

aws/import_aws_iot_thing_type_test.go Outdated
"github.com/hashicorp/terraform/helper/resource"
)

func TestAccAWSIotThingTypeTest_importBasic(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: TestAccAWSIotThingTypeTest_ -> TestAccAWSIotThingType_

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't even know where these XTest_ come from, it's not even copy-pasta 🤔

aws/resource_aws_iot_thing_type_test.go Outdated
"github.com/hashicorp/terraform/terraform"
)

func TestAccAWSIotThingTypeTest_basic(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: as above - we don't need Test twice in the name 😉

aws/resource_aws_iot_thing_type_test.go Outdated
})
}

func TestAccAWSIotThingTypeTest_full(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: as above - we don't need Test twice in the name 😉

aws/resource_aws_iot_thing_type_test.go Outdated
})
}

func testAccCheckAWSIotThingTypeTestDestroy(s *terraform.State) error {
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: as above - we don't need Test twice in the name 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks!

aws/resource_aws_iot_thing_type_test.go Outdated
return nil
}

func testAccAWSIotThingTypeTest_basic(rName int) string {
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: We tend to call this Config rather than Test.

aws/resource_aws_iot_thing_type_test.go Outdated
`, rName)
}

func testAccAWSIotThingTypeTest_full(rName int) string {
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: We tend to call this Config rather than Test.

aws/resource_aws_iot_thing_type_test.go Outdated
`, rName)
}

func testAccAWSIotThingTypeTest_fullUpdated(rName int) string {
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: We tend to call this Config rather than Test.

@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 20, 2018
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 20, 2018
@Ninir
Copy link
Contributor Author

Ninir commented Feb 20, 2018

@radeksimko Just fixed nitpicks, thanks for the review.

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSIotThingType_'    
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSIotThingType_ -timeout 120m
=== RUN   TestAccAWSIotThingType_importBasic
--- PASS: TestAccAWSIotThingType_importBasic (328.60s)
=== RUN   TestAccAWSIotThingType_basic
--- PASS: TestAccAWSIotThingType_basic (325.46s)
=== RUN   TestAccAWSIotThingType_full
--- PASS: TestAccAWSIotThingType_full (340.39s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	994.497s

@Ninir Ninir merged commit 8ff7a12 into hashicorp:master Feb 20, 2018
@Ninir Ninir deleted the f-iot-thing-type branch February 20, 2018 15:52
Ninir added a commit that referenced this pull request Feb 20, 2018
@Ninir
Update CHANGELOG for #3302
3452102
@bflad
Copy link
Contributor

bflad commented Feb 27, 2018

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

@ghost
Copy link

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

@radeksimko radeksimko radeksimko approved these changes

@bflad bflad Awaiting requested review from bflad

Assignees
No one assigned
Labels
enhancement Requests to existing resources that expand the functionality or scope. new-resource Introduces a new resource. service/iot Issues and PRs that pertain to the iot service. size/L Managed by automation to categorize the size of a PR.
Projects
None yet
Milestone
v1.10.0
Development

Successfully merging this pull request may close these issues.
4 participants
@Ninir @cemo @bflad @radeksimko