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

Exposed ignore_missing_vnet_service_endpoint attribute #2056

Merged
merged 10 commits into from
Oct 12, 2018

Conversation

WodansSon
Copy link
Collaborator

Fixed the issue for the ignore_missing_vnet_service_endpoint attribute, turns out it's an async operation for Postgre SQL. Currently have many customers asking for this functionality.

@@ -84,7 +85,7 @@ The following arguments are supported:

* `subnet_id` - (Required) The ID of the subnet that the PostgreSQL server will be connected to.

~> **NOTE:** Due to [a bug in the Azure API](https://github.com/Azure/azure-rest-api-specs/issues/3719) this resource currently doesn't expose the `ignore_missing_vnet_service_endpoint` field and defaults this to `false`. Terraform will check during the provisioning of the Virtual Network Rule that the Subnet contains the Service Rule to verify that the Virtual Network Rule can be created.
* `ignore_missing_vnet_service_endpoint` - (Optional) Create the virtual network rule before the subnet has the virtual network service endpoint enabled. The default value is false.
Copy link
Contributor

Choose a reason for hiding this comment

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

Create the virtual network rule before the subnet has the virtual network service endpoint enabled. The default value is false.

I think we could phrase this better to match the syntax of the other fields:

Should the Virtual Network Rule be created before the Subnet has the Virtual Network Service Endpoint enabled? Defaults to `false`

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How about I copy it from the API docs directly?

Create firewall rule before the virtual network has vnet service endpoint enabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure - but we should still match the language used across the other resources, which prefixes things with Should [xxx] be applied/enabled etc?

@tombuildsstuff tombuildsstuff modified the milestones: 1.17.0, 1.18.0 Oct 11, 2018
Copy link

@JunyiYi JunyiYi left a comment

Choose a reason for hiding this comment

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

Thanks @jeffreyCline for the contribution. I just left some small comments in the code, other parts looks good to me.

@tombuildsstuff tombuildsstuff modified the milestones: 1.18.0, 1.17.0 Oct 11, 2018
@WodansSon
Copy link
Collaborator Author

image

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

@tombuildsstuff tombuildsstuff merged commit e39ecc1 into master Oct 12, 2018
@tombuildsstuff tombuildsstuff deleted the b-postgre-ignore-missing branch October 12, 2018 19:47
tombuildsstuff added a commit that referenced this pull request Oct 12, 2018
@ghost
Copy link

ghost commented Mar 6, 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. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants