-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
add new resource & data source: azurerm_netapp_account
#4416
Conversation
Ping. Pong? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR, i've left some comments inline that need to be addressed before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the revisions @neil-yechenwei! looking much better.
It looks like there can be only one active_directory
block:
------- Stdout: -------
=== RUN TestAccAzureRMNetAppAccount_update
=== PAUSE TestAccAzureRMNetAppAccount_update
=== CONT TestAccAzureRMNetAppAccount_update
--- FAIL: TestAccAzureRMNetAppAccount_update (83.58s)
testing.go:569: Step 1 error: errors during apply:
Error: Error creating NetApp Account "acctest-NetAppAccount-191008004136081477" (Resource Group "acctestRG-191008004136081477"): netapp.AccountsClient#CreateOrUpdate: Failure sending request: StatusCode=400 -- Original Error: Code="ErrorUpsertingActiveDirectory" Message="Only one active directory allowed"
on /opt/teamcity-agent/temp/buildTmp/tf-test307630958/main.tf line 7:
(source code not available)
Thanks for the comments @katbyte . After checked for this resource in Azure portal, it only supports one active directory. So I added attribute "MaxItems: 1". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the revisions @neil-yechenwei,
In addition to the comments i've left inline the update test is still broken:
------- Stdout: -------
=== RUN TestAccAzureRMNetAppAccount_update
=== PAUSE TestAccAzureRMNetAppAccount_update
=== CONT TestAccAzureRMNetAppAccount_update
--- FAIL: TestAccAzureRMNetAppAccount_update (91.27s)
testing.go:569: Step 1 error: errors during apply:
Error: Error creating NetApp Account "acctest-NetAppAccount-191009061916948164" (Resource Group "acctestRG-netapp-191009061916948164"): netapp.AccountsClient#CreateOrUpdate: Failure sending request: StatusCode=400 -- Original Error: Code="ErrorUpsertingActiveDirectory" Message="Only one active directory allowed"
there could be a default one created if you don't specify one??
Hi @katbyte , Thank you for the comments, I've updated code, thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pushing those changes - I've taken a look through and left some extra comments in addition to @katbyte's - but if we can fix those up and the tests pass this should otherwise be good.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pushing those changes - I've taken a look through and left some extra comments in addition to @katbyte's - but if we can fix those up and the tests pass this should otherwise be good.
Thanks!
@tombuildsstuff , I've updated code by your comments. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing those comments @neil-yechenwei,
I've answered some of your questions and let a couple more comments inline.
We don't need to write acctests to test validation functions, and if we just use the validation.stringmatch
we don't need a test at all
@katbyte ,Thanks for your comments. I've updated code by your comments. |
We're using ANF a lot more and are eager for this feature. Near as I can tell, @katbyte and @tombuildsstuff , @neil-yechenwei has implemented your latest round of changes from your reviews. Anything else blocking? I'm not an allowed reviewer, obviously. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the revisions @neil-yechenwei, this LGTM now!
password = "aduserpwd" | ||
smb_server_name = "SMBSERVER" | ||
dns_servers = ["1.2.3.4"] | ||
domain = "westcentralus.com" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
confirmed this, when you try and set two different ones you get:
Error: Error waiting for creation of NetApp Account "acctest-NetAppAccount-191114162218570150" (Resource Group "acctestRG-netapp-191114162218570150"): Code="InternalServerError" Message="Error creating ActiveDirectory."
🤷♀
azurerm_netapp_account
azurerm_netapp_account
azurerm_netapp_account
@katbyte , I am not sure what you mean about "confirmed this, when you try and set two different ones you get:". Are you trying to test for changing the active directory from A to B, right? If yes, it seems that it works fine from my side. If not, could you share me the code example? Thanks. |
This has been released in version 1.37.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 = "~> 1.37.0"
}
# ... other configuration ... |
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! |
This PR is code implement for issue #4417