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_storagegateway_working_storage #5285

Merged
merged 2 commits into from
Jul 27, 2018
Merged

New Resource: aws_storagegateway_working_storage #5285

merged 2 commits into from
Jul 27, 2018

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Jul 20, 2018

Built on #5279
Reference #943

Changes proposed in this pull request:

  • New Resource: aws_storagegateway_working_storage

Output from acceptance testing:

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

@bflad bflad added new-resource Introduces a new resource. service/storagegateway Issues and PRs that pertain to the storagegateway service. labels Jul 20, 2018
@bflad bflad requested a review from a team July 20, 2018 22:18
@ghost ghost added the size/XL Managed by automation to categorize the size of a PR. label Jul 20, 2018
@bflad bflad mentioned this pull request Jul 20, 2018
Closed
appilon
appilon approved these changes Jul 27, 2018
View reviewed changes
Copy link
Contributor

@appilon appilon left a comment

Choose a reason for hiding this comment

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

Overall approved! I had some minor code suggestions but nothing I felt strongly about.

aws/data_source_aws_storagegateway_local_disk.go

for _, disk := range output.Disks {
if aws.StringValue(disk.DiskPath) == diskPath {
matchingDisks = append(matchingDisks, disk)
Copy link
Contributor

Choose a reason for hiding this comment

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

It appears finding more than 1 disk is an error? Perhaps refactor to single match and break when you find it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A disk path is a unique identifier on a system, e.g. /dev/sdb

aws/data_source_aws_storagegateway_local_disk.go
return errors.New("multiple results found for query, try adjusting your search criteria")
}

disk := matchingDisks[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

again do we foresee multiple matches being something that comes later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We would create a separate pluralized data source (e.g. aws_storagegateway_local_disks) in that case

aws/resource_aws_storagegateway_working_storage.go
return nil
}

found := false
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick var found bool would begin false... I'm okay either way :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Go newcomers might be confused by variable type zero-value behavior -- explicit > implicit IMO since it doesn't cost any extra lines of code. 👍

aws/resource_aws_storagegateway_working_storage.go
}

func decodeStorageGatewayWorkingStorageID(id string) (string, string, error) {
// id = arn:aws:storagegateway:us-east-1:123456789012:gateway/sgw-12345678:pci-0000:03:00.0-scsi-0:0:0:0
Copy link
Contributor

Choose a reason for hiding this comment

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

remnant from development?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm showing these for future travelers since the logic might be seen as non-trivial

aws/resource_aws_storagegateway_working_storage.go
if err != nil {
return "", "", idFormatErr
}
// gatewayARNAndDisk.Resource = gateway/sgw-12345678:pci-0000:03:00.0-scsi-0:0:0:0
Copy link
Contributor

Choose a reason for hiding this comment

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

development remnant?

aws/resource_aws_storagegateway_working_storage.go
if len(resourceParts) != 2 {
return "", "", idFormatErr
}
// resourceParts = ["gateway/sgw-12345678", "pci-0000:03:00.0-scsi-0:0:0:0"]
Copy link
Contributor

Choose a reason for hiding this comment

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

remnant?

aws/resource_aws_storagegateway_working_storage_test.go
"github.com/hashicorp/terraform/terraform"
)

func TestDecodeStorageGatewayWorkingStorageID(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.

👍 for unit tests!

aws/resource_aws_storagegateway_working_storage_test.go
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
// Storage Gateway API does not support removing working storages
Copy link
Contributor

Choose a reason for hiding this comment

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

I have no clue what this service is, I'm assuming we aren't leaving around costly infrastructure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct 😄 Storage Gateway caches, upload buffers, and working storages are all associated with a gateway and deleted when the gateway is deleted.

@bflad bflad added this to the v1.30.0 milestone Jul 27, 2018
@bflad bflad force-pushed the f-aws_storagegateway_working_storage branch from 13729f9 to fd739fb Compare July 27, 2018 19:27
@ghost ghost added the size/XL Managed by automation to categorize the size of a PR. label Jul 27, 2018
@bflad
Copy link
Contributor Author

bflad commented Jul 27, 2018

Rebased on top of master to resolve conflicts, no surprises from CI, merging!

@bflad bflad merged commit 59ab5bd into master Jul 27, 2018
@bflad bflad deleted the f-aws_storagegateway_working_storage branch July 27, 2018 19:32
@bflad bflad mentioned this pull request Jul 27, 2018
Closed
bflad added a commit that referenced this pull request Jul 27, 2018
@bflad
Update CHANGELOG for #5279 and #5285
e9d7a37
bflad added a commit that referenced this pull request Jul 27, 2018
@bflad
Update CHANGELOG for #5284 and fix ordering for #5285
34a7ab1
@bflad
Copy link
Contributor Author

bflad commented Aug 2, 2018

This has been released in version 1.30.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 4, 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 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@appilon appilon appilon approved these changes

Assignees
No one assigned
Labels
new-resource Introduces a new resource. service/storagegateway Issues and PRs that pertain to the storagegateway service. size/XL Managed by automation to categorize the size of a PR.
Projects
None yet
Milestone
v1.30.0
Development

Successfully merging this pull request may close these issues.
2 participants
@bflad @appilon