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_servicequotas_service_quota #9192

Merged
merged 3 commits into from
Jul 16, 2019
Merged

New Resource: aws_servicequotas_service_quota #9192

merged 3 commits into from
Jul 16, 2019

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Jul 1, 2019

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Replacing #9121 due to bad rebase...
Closes #9118

Release note for CHANGELOG:

* **New Resource:** `aws_servicequotas_service_quota`

Output from acceptance testing (requesting increase of t3.nano and t3.micro limits in testing account):

export SERVICEQUOTAS_INCREASE_ON_CREATE_QUOTA_CODE=L-55EB784E
export SERVICEQUOTAS_INCREASE_ON_CREATE_SERVICE_CODE=ec2
export SERVICEQUOTAS_INCREASE_ON_CREATE_VALUE=25
export SERVICEQUOTAS_INCREASE_ON_UPDATE_QUOTA_CODE=L-0F0ED007
export SERVICEQUOTAS_INCREASE_ON_UPDATE_SERVICE_CODE=ec2
export SERVICEQUOTAS_INCREASE_ON_UPDATE_VALUE=25

--- PASS: TestAccAwsServiceQuotasServiceQuota_basic (13.83s)
--- PASS: TestAccAwsServiceQuotasServiceQuota_Value_IncreaseOnCreate (12.52s)
--- PASS: TestAccAwsServiceQuotasServiceQuota_Value_IncreaseOnUpdate (20.00s)

Output from acceptance testing in AWS GovCloud (US):

--- SKIP: TestAccAwsServiceQuotasServiceQuota_basic (2.51s)
    resource_aws_servicequotas_service_quota_test.go:138: skipping acceptance testing: RequestError: send request failed
        caused by: Post https://servicequotas.us-gov-west-1.amazonaws.com/: dial tcp: lookup servicequotas.us-gov-west-1.amazonaws.com: no such host

@bflad
New Resource: aws_servicequotas_service_quota
4edb353 
Reference: #9118

Output from acceptance testing (requesting increase of t3.nano and t3.micro limits in testing account):

```
export SERVICEQUOTAS_INCREASE_ON_CREATE_QUOTA_CODE=L-55EB784E
export SERVICEQUOTAS_INCREASE_ON_CREATE_SERVICE_CODE=ec2
export SERVICEQUOTAS_INCREASE_ON_CREATE_VALUE=25
export SERVICEQUOTAS_INCREASE_ON_UPDATE_QUOTA_CODE=L-0F0ED007
export SERVICEQUOTAS_INCREASE_ON_UPDATE_SERVICE_CODE=ec2
export SERVICEQUOTAS_INCREASE_ON_UPDATE_VALUE=25

--- PASS: TestAccAwsServiceQuotasServiceQuota_basic (13.83s)
--- PASS: TestAccAwsServiceQuotasServiceQuota_Value_IncreaseOnCreate (12.52s)
--- PASS: TestAccAwsServiceQuotasServiceQuota_Value_IncreaseOnUpdate (20.00s)
```

Output from acceptance testing in AWS GovCloud (US):

```
--- SKIP: TestAccAwsServiceQuotasServiceQuota_basic (2.51s)
    resource_aws_servicequotas_service_quota_test.go:138: skipping acceptance testing: RequestError: send request failed
        caused by: Post https://servicequotas.us-gov-west-1.amazonaws.com/: dial tcp: lookup servicequotas.us-gov-west-1.amazonaws.com: no such host
```
@bflad bflad added new-resource Introduces a new resource. service/servicequotas Issues and PRs that pertain to the servicequotas service. labels Jul 1, 2019
@bflad bflad requested a review from a team July 1, 2019 19:19
@ghost ghost added provider Pertains to the provider itself, rather than any interaction with AWS. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. documentation Introduces or discusses updates to documentation. size/XL Managed by automation to categorize the size of a PR. labels Jul 1, 2019
This was referenced Jul 1, 2019
Closed
Closed
nywilken
nywilken reviewed Jul 15, 2019
View reviewed changes
aws/provider.go
@@ -681,6 +681,7 @@ func Provider() terraform.ResourceProvider {
"aws_service_discovery_private_dns_namespace": resourceAwsServiceDiscoveryPrivateDnsNamespace(),
"aws_service_discovery_public_dns_namespace": resourceAwsServiceDiscoveryPublicDnsNamespace(),
"aws_service_discovery_service": resourceAwsServiceDiscoveryService(),
"aws_servicequotas_service_quota": resourceAwsServiceQuotasServiceQuota(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I know we generally use the AWS service name for the resource name, but this name has a bit of a stutter. What do you about just using aws_service_quotas?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is noted in the Contributing Guide with the following item:

Naming: Resources should be named aws_<service>_<name> where service is the AWS short service name and name is a short, preferably single word, description of the resource. Use _ as a separator.

What isn't included in the guide is the reasoning:

  • It helps operators and maintainers easier find resources by AWS service or configure Customized Endpoints using the service name that is part of the resource name. There will be other resources with this service: Support Managing Service Quotas Templates #9119
  • Prevents conflicting naming issues should AWS release other "quota" related services/resources in the future
  • Potentially sets up the resources for easier auto-generation in the future

We likely should include some/all of this rationale either in the contributing or maintaining guides for easier reference. I'll raise a PR for the contributing guide to start.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See also: #9347 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense. Thanks for rationale. I found the existing data sources after leaving the comment and quickly thought to rescind this comment. Especially, after thinking more about "Prevents conflicting naming issues should AWS release other "quota" related services/resources in the future". The contributing guide update looks good 👍

@bflad bflad mentioned this pull request Jul 16, 2019
Merged
nywilken
nywilken approved these changes Jul 16, 2019
View reviewed changes
Copy link
Contributor

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

This looks good to me. Nicely done. I left a nit and a couple of questions. Nothing that would prevent this from being merged so I am going to approve the PR as it is.

aws/resource_aws_servicequotas_service_quota.go Outdated Show resolved Hide resolved
aws/resource_aws_servicequotas_service_quota_test.go Outdated Show resolved Hide resolved
website/docs/r/servicequotas_service_quota.html.markdown

## Import

~> *NOTE* This resource does not require explicit import and will assume management of an existing service quota on Terraform resource creation.
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

aws/resource_aws_servicequotas_service_quota.go
return fmt.Errorf("error getting Service Quotas Service Quota (%s): empty result", d.Id())
}

if value > aws.Float64Value(output.Quota.Value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Providing a value less than the current quota value would result in a noop and exit successfully, correct? If so should there be an error message to the operator that the provided value is less than the currently set quota?

Copy link
Contributor Author

@bflad bflad Jul 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Correct. Terraform should show automatically show perpetual difference in this case. 😄 Correction! The call to the API will fail, which is no different than us providing our own error message. 👍

@bflad bflad added this to the v2.20.0 milestone Jul 16, 2019
@bflad bflad merged commit 937b804 into master Jul 16, 2019
@bflad bflad deleted the f-servicequotas-resource branch July 16, 2019 19:14
bflad added a commit that referenced this pull request Jul 16, 2019
@bflad
Update CHANGELOG for #9192
6a6a759
@ghost
Copy link

ghost commented Nov 2, 2019

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 Nov 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nywilken nywilken nywilken approved these changes

Assignees
No one assigned
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/servicequotas Issues and PRs that pertain to the servicequotas 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
Milestone
v2.20.0
Development

Successfully merging this pull request may close these issues.

Support Managing Service Quotas
2 participants
@bflad @nywilken