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

docs/CONTRIBUTING: New Region #3146

Merged
merged 2 commits into from
Jan 26, 2018
Merged

docs/CONTRIBUTING: New Region #3146

merged 2 commits into from
Jan 26, 2018

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Jan 26, 2018

From PR review of #3142

@bflad bflad added the documentation Introduces or discusses updates to documentation. label Jan 26, 2018
@kwerey
Copy link
Contributor

kwerey commented Jan 26, 2018

Hey, a minor other bit of feedback on these guidelines as a newbie: the example for running acceptance tests looks like it comes from a time when the provider lived in the main terraform repo:

$ make testacc TEST=./builtin/providers/azurerm TESTARGS='-run=TestAccAzureRMPublicIpStatic_update'
==> Checking that code complies with gofmt requirements..

It's no big deal to find an aws alternative, but updating the example to something that'll work out the box with the AWS provider would lower the barrier to entry a touch by saving folks from having to break the command apart and parse how the directory structure's changed!

@bflad
Copy link
Contributor Author

bflad commented Jan 26, 2018

That is excellent feedback! I will submit a PR to get the documentation updated. Thank you!

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.

I hope this list won't grow much over time 😅

What I usually do is grep eu-west-3 or something like that and find all the places where I need to add new region.

Generally speaking - and that is a blocker - I'd rather not see a region in the map at all rather than empty string and have Terraform error out in such case.

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.

@bflad
Copy link
Contributor Author

bflad commented Jan 26, 2018

@radeksimko you're absolutely right 👍 we should have proper handling for missing map entries (friendly error not crash). I think we identified at least one place where this definitely is an issue and I'll submit an issue for it and others if necessary. I have updated the documentation here say if available rather than adding an empty mapping entry.

@bflad bflad merged commit d928613 into master Jan 26, 2018
@bflad bflad deleted the d-new-regions branch January 26, 2018 15:11
@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.
Labels
documentation Introduces or discusses updates to documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants