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

New resource - azurerm_integration_service_environment #7763

Merged
merged 34 commits into from
Aug 11, 2020
Merged

New resource - azurerm_integration_service_environment #7763

merged 34 commits into from
Aug 11, 2020

Conversation

jbinko
Copy link
Contributor

@jbinko jbinko commented Jul 15, 2020

Support for Integration Service Environments
See spec in the issue above.

It contains:

  • Documentation
  • Resource: azurerm_integration_service_environment
  • Unit Tests
  • Acc Tests
  • Hack for delete operation - see comments in source code.
  • Helper code

make test TEST='github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/logic/parse' TESTARGS='-run=TestIntegrationServiceEnvironmentId'
make test TEST='github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/logic/validate' TESTARGS='-run=TestIntegrationServiceEnvironmentName'

make acctests SERVICE='logic' TESTARGS='-run=TestAccAzureRMIntegrationServiceEnvironment_basic' TESTTIMEOUT='600m'
make acctests SERVICE='logic' TESTARGS='-run=TestAccAzureRMIntegrationServiceEnvironment_requiresImport' TESTTIMEOUT='600m'
make acctests SERVICE='logic' TESTARGS='-run=TestAccAzureRMIntegrationServiceEnvironment_update' TESTTIMEOUT='600m'

Please provide feedback.

(fixes #7394)

Copy link
Collaborator

@WodansSon WodansSon left a comment

Choose a reason for hiding this comment

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

@jbinko, thanks so much for this! This mostly looks good but I added a few comments, which are mostly fairly minor, but it is looking really good and I appreciate all the effort you put in to creating the PR! This is looking awesome! Once the changes are push we can get this merged into main! Thanks again! 🚀

azurerm/internal/services/logic/client/client.go Outdated Show resolved Hide resolved
website/docs/r/integration_service_environment.markdown Outdated Show resolved Hide resolved
website/docs/r/integration_service_environment.markdown Outdated Show resolved Hide resolved
website/docs/r/integration_service_environment.markdown Outdated Show resolved Hide resolved
website/docs/r/integration_service_environment.markdown Outdated Show resolved Hide resolved
website/docs/r/integration_service_environment.markdown Outdated Show resolved Hide resolved
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.

spotted a couple of things whilst passing through

@jbinko
Copy link
Contributor Author

jbinko commented Jul 17, 2020

I agree with all the comments above. Thank you for review.
I’m not just sure about proposal merging capacity and SKU together for Developer sku.
We need sku_name and capacity properties for Premium use case anyway. Correct?
For Developer use case you need specify sku_name as well and you either skip capacity property (Default is 0)
Or you need to make sure capacity is 0 and if not - TF will report it before provisioning.

In my opinion, introducing additional artificial sku name Developer_0 will
Increase confusion and increase code complexity because of more options/special cases.
Not sure also what would be the advantage of such sku. What are we trying to address?
Don’t get me wrong, I just think I might be missing something.
@WodansSon: Can you please describe in more details the idea? Thank you.

@jackofallops jackofallops added this to the v2.21.0 milestone Jul 23, 2020
@WodansSon
Copy link
Collaborator

WodansSon commented Jul 24, 2020

I agree with all the comments above. Thank you for review.
I’m not just sure about proposal merging capacity and SKU together for Developer sku.
We need sku_name and capacity properties for Premium use case anyway. Correct?
For Developer use case you need specify sku_name as well and you either skip capacity property (Default is 0)
Or you need to make sure capacity is 0 and if not - TF will report it before provisioning.

In my opinion, introducing additional artificial sku name Developer_0 will
Increase confusion and increase code complexity because of more options/special cases.
Not sure also what would be the advantage of such sku. What are we trying to address?
Don’t get me wrong, I just think I might be missing something.
@WodansSon: Can you please describe in more details the idea? Thank you.

@jbinko, sorry for the late reply... the reason we flatten the sku block into a underscore delimited string is because we had many customers complaining that it was difficult to automate the sku block in their automation scripts and that a structured string was a much easier way for them to manipulate and update while running their scripts against the resource via Terraform. For an implementation example you can take a look at the postgresql_server_resource.go resource. As such, the sku_name delimited string is the new standard moving forward for all resources to help our customers automate Terraform in their environments. Feel free to ask more questions if I didn't explain it clearly enough or if you still have questions around why the two attributes need to be combined into a single sku_name attribute.

Oh one other thing I forgot to mention, in the docs, be sure to mention the order they need to be in, like the below example from the PostgreSQL Server documentation:

sku_name - (Required) Specifies the SKU Name for this PostgreSQL Server. The name of the SKU, follows the tier + family + cores pattern (e.g. B_Gen4_1, GP_Gen5_8). For more information see the product documentation.

@katbyte
Copy link
Collaborator

katbyte commented Jul 24, 2020

@jbinko - you can also validate the sku_name with regex/a custom validation function that splits it apart. We spent some effort earlier this year to move resources to sku_name when reasonable/made sense and this seems like one of those times.

@jbinko
Copy link
Contributor Author

jbinko commented Jul 28, 2020

@WodansSon, @katbyte: Got it. Now I understand. I changed sku_name as you proposed. Code should be ready for another review. Let me know what do you think.

@ghost ghost removed the waiting-response label Jul 28, 2020
@katbyte katbyte modified the milestones: v2.21.0, v2.22.0 Jul 31, 2020
@WodansSon
Copy link
Collaborator

@jbinko, thanks for that... I will take a look at this tomorrow morning my time (PST), I've been out for the last few days for eye surgery...

@katbyte katbyte modified the milestones: v2.22.0, v2.23.0 Aug 7, 2020
Copy link
Collaborator

@WodansSon WodansSon left a comment

Choose a reason for hiding this comment

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

@jbinko, thanks so much for pushing those changes this LGTM now! 🚀

Copy link
Collaborator

@WodansSon WodansSon left a comment

Choose a reason for hiding this comment

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

@jbinko, one thing really quick... super minor, but I've left 5 suggestions to rename the resource group in the test cases. If you can merge those I will merge this PR into master and we can get it into this release! 🚀

@jbinko
Copy link
Contributor Author

jbinko commented Aug 11, 2020

Thanks @WodansSon ! Sure, I merged your suggestions. Done.

@katbyte katbyte merged commit 07b3926 into hashicorp:master Aug 11, 2020
katbyte added a commit that referenced this pull request Aug 11, 2020
@ghost
Copy link

ghost commented Aug 13, 2020

This has been released in version 2.23.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.23.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Sep 11, 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 and limited conversation to collaborators Sep 11, 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.

Support for Integration Service Environments
5 participants