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

azurerm_postgresql_firewall_rule - add validation for start_ip_address and end_ip_address #8963

Merged
merged 2 commits into from
Nov 5, 2020

Conversation

ritesh-modi
Copy link
Contributor

Added postgres firewall start and end IP address validation

Acceptance tests details

riteshmodi@MININT-3FOKASG terraform-provider-azurerm % make acctests SERVICE=postgres TESTARGS='-run=TestAccAzureRMPostgreSQLFirewallRule' TESTTIMEOUT='120m'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./azurerm/internal/services/postgres/tests/ -run=TestAccAzureRMPostgreSQLFirewallRule -timeout 120m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN TestAccAzureRMPostgreSQLFirewallRule_basic
=== PAUSE TestAccAzureRMPostgreSQLFirewallRule_basic
=== RUN TestAccAzureRMPostgreSQLFirewallRule_requiresImport
=== PAUSE TestAccAzureRMPostgreSQLFirewallRule_requiresImport
=== CONT TestAccAzureRMPostgreSQLFirewallRule_basic
=== CONT TestAccAzureRMPostgreSQLFirewallRule_requiresImport
--- PASS: TestAccAzureRMPostgreSQLFirewallRule_basic (264.14s)
--- PASS: TestAccAzureRMPostgreSQLFirewallRule_requiresImport (269.61s)
PASS
ok github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/postgres/tests 269.659s

@ritesh-modi
Copy link
Contributor Author

Hi @tombuildsstuff , let me know if this PR looks good.

Thanks in advance, Ritesh

Copy link
Collaborator

@magodo magodo left a comment

Choose a reason for hiding this comment

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

@ritesh-modi Thank you for this PR! 👍

Overall It LGTM, whilst can we split this PR into two? One for adding the validation for the postgres firewall resource, and the other for supporting the same size kubernetes cluster node pool.

@ritesh-modi
Copy link
Contributor Author

Hello @magodo Thanks for letting me know about changes required.

I have done the needed changes and please help in closing the PR.

@magodo
Copy link
Collaborator

magodo commented Nov 4, 2020

@ritesh-modi
LGTM now!
Whilst I'm curious what is the motivation for this PR? And also want to ensure that the API doesn't works for ipv6.

@ritesh-modi
Copy link
Contributor Author

@magodo, no other motivation apart from learning and contributing to open-source. Also, yes the resource expect ipv4 address only.

image

@magodo
Copy link
Collaborator

magodo commented Nov 4, 2020

@ritesh-modi Thanks 👍

@magodo magodo added this to the v2.35.0 milestone Nov 4, 2020
@magodo magodo changed the title Added postgres firewall start and end IP address validation azurerm_postgresql_firewall_rule - add validation for start_ip_address and end_ip_address Nov 4, 2020
@magodo
Copy link
Collaborator

magodo commented Nov 4, 2020

@ritesh-modi One thing I overlooked is that i noticed you are using the validation function from the provider code base. However, we are now prefer using the ones from terraform plugin sdk if available. So in your case, you might want to use validation.IsIPv4Address instead. Sorry for the late comment...

@ritesh-modi
Copy link
Contributor Author

Sure @magodo, I have done this change. thanks for informing !

@magodo magodo merged commit a2a7f3e into hashicorp:master Nov 5, 2020
@magodo
Copy link
Collaborator

magodo commented Nov 5, 2020

image

magodo added a commit that referenced this pull request Nov 5, 2020
@ritesh-modi ritesh-modi deleted the postgres branch November 5, 2020 12:10
@ghost
Copy link

ghost commented Nov 5, 2020

This has been released in version 2.35.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.35.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Dec 5, 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. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Dec 5, 2020
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.

3 participants