new resource `azurerm_synapse_sql_pool` #8095

njuCZ · 2020-08-12T10:07:53Z

the third resource of #7406

it needs to take a long time to run test for PointInTime restore (8h according to the doc) and recovery (1 day), so I don't add test case for these two create mode. I have tested it offline and there is no problem

(fixes #7406)

katbyte

Thanks @njuCZ - aside from a few minor comments this looks pretty good

azurerm/internal/services/synapse/synapse_sql_pool_resource.go

website/docs/r/synapse_sql_pool.html.markdown

azurerm/internal/services/synapse/synapse_sql_pool_resource.go

njuCZ · 2020-08-13T02:44:14Z

@katbyte I have refined my PR, could you please have a look again?

Requested changes have been made.

WodansSon

@njuCZ, thanks for pushing the requested changes by @katbyte ... LGTM! 🚀

tombuildsstuff

hi @njuCZ

Thanks for this PR.

I've taken a look through here and left some comments inline - but this is off to a good start. If we can fix up the comments then we should be able to take another look through here.

Thanks!

azurerm/internal/services/synapse/synapse_sql_pool_resource.go

tombuildsstuff · 2020-08-20T10:25:23Z

azurerm/internal/services/synapse/synapse_sql_pool_resource.go

+			"data_encrypted": {
+				Type:     schema.TypeBool,
+				Optional: true,
+			},


this looks like a value that'll change in the future:

a) is this defaulted off?
b) is this going to change to on in the future?
c) is this not a ForceNew field like in many other API's?

a) for now, it's default "off"
b) I am sorry that I don't know whether it will change default value in the future
c) it's a updatable field. I don't see similiar fields in other API?

azurerm/internal/services/synapse/synapse_sql_pool_resource.go

tombuildsstuff · 2020-08-20T10:41:06Z

website/docs/r/synapse_sql_pool.html.markdown

+
+* `create_mode` - (Optional) Specifies how to create the Sql Pool. Valid values are: `Default`, `Recovery` or `PointInTimeRestore`. Must be `Default` to create a new database. Defaults to `Default`.
+
+* `collation` - (Optional) The name of the collation to use with this pool. Applies only if `create_mode` is `Default`.  Azure default is `SQL_LATIN1_GENERAL_CP1_CI_AS`. Changing this forces a new resource to be created.


we should default this within Terraform too:

Suggested change

* `collation` - (Optional) The name of the collation to use with this pool. Applies only if `create_mode` is `Default`. Azure default is `SQL_LATIN1_GENERAL_CP1_CI_AS`. Changing this forces a new resource to be created.

* `collation` - (Optional) The name of the collation to use with this pool, only applicable when `create_mode` is set to `Default`. Defaults to `SQL_LATIN1_GENERAL_CP1_CI_AS`. Changing this forces a new resource to be created.

we could not add default value here, the reason is as below

azurerm/internal/services/sql/parse/sql_database.go

tombuildsstuff · 2020-08-20T10:43:12Z

azurerm/internal/services/synapse/synapse_sql_pool_resource.go

+				Type:     schema.TypeString,
+				Optional: true,
+				Computed: true,
+				ForceNew: true,


(as below) this should probably be defaulted

I believe we've also got validation for collations somewhere in the codebase we could use here

I have added validation. But I think this field should not have default value. Because in "recovery" or "restore" createmode, the "collation" is the same with the sourceDatabase, but not what user input. If there is a default value, it might cause diff in this case.

azurerm/internal/services/synapse/synapse_sql_pool_resource.go

njuCZ · 2020-08-24T05:39:37Z

@tombuildsstuff I have modifed this PR, could you please have a look again?

jackofallops · 2020-09-10T10:14:48Z

Hi @njuCZ - Is there a reason this is leveraging/updating sql rather than the newer mssql ?

katbyte

Thanks @njuCZ - LGTM 👍

njuCZ · 2020-09-11T04:59:27Z

@jackofallops sorry that I understand the relation between sql and mssql just now. Since this PR has been merged, and only parse function and validation function from sql package is used. I will change to use functions from mssql. This is not breaking change and I will submit follow up PR to do this #8434 . Thanks for your suggestion

ghost · 2020-09-17T17:46:31Z

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

ghost · 2020-10-11T14:50:31Z

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 added size/XXL documentation labels Aug 12, 2020

katbyte previously requested changes Aug 12, 2020

View reviewed changes

katbyte added new-resource service/synapse labels Aug 12, 2020

katbyte added this to the v2.23.0 milestone Aug 12, 2020

tombuildsstuff modified the milestones: v2.23.0, v2.24.0 Aug 13, 2020

WodansSon approved these changes Aug 20, 2020

View reviewed changes

njuCZ force-pushed the synapse_sql_pool branch from 39aa39f to 67edd50 Compare August 20, 2020 07:16

tombuildsstuff suggested changes Aug 20, 2020

View reviewed changes

tombuildsstuff modified the milestones: v2.24.0, v2.25.0 Aug 20, 2020

tombuildsstuff added the waiting-response label Aug 20, 2020

njuCZ added 5 commits August 21, 2020 10:56

new resource "azurerm_synapse_sql_pool"

d958f01

add validation for source_database_id

888f8a9

update

4afe687

update according to KT's suggestion

fc1c651

modify according to Tom's suggestion

75c3455

njuCZ force-pushed the synapse_sql_pool branch from 67edd50 to 75c3455 Compare August 24, 2020 05:10

ghost removed the waiting-response label Aug 24, 2020

tombuildsstuff modified the milestones: v2.25.0, v2.26.0 Aug 27, 2020

tombuildsstuff modified the milestones: v2.26.0, v2.27.0 Sep 3, 2020

jackofallops added the waiting-response label Sep 10, 2020

jackofallops modified the milestones: v2.27.0, v2.28.0 Sep 10, 2020

katbyte approved these changes Sep 11, 2020

View reviewed changes

katbyte merged commit 87114b2 into hashicorp:master Sep 11, 2020

katbyte added a commit that referenced this pull request Sep 11, 2020

update CHANGELOG.md for #8095

928462a

njuCZ mentioned this pull request Sep 11, 2020

azurerm_synapse_sql_pool - leverage newer version mssql #8434

Merged

ghost removed the waiting-response label Sep 11, 2020

WodansSon pushed a commit that referenced this pull request Sep 11, 2020

follow up of PR #8095

d7aad8b

ghost locked as resolved and limited conversation to collaborators Oct 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new resource `azurerm_synapse_sql_pool` #8095

new resource `azurerm_synapse_sql_pool` #8095

njuCZ commented Aug 12, 2020 •

edited by katbyte

Loading

katbyte left a comment

njuCZ commented Aug 13, 2020

WodansSon left a comment

tombuildsstuff left a comment

tombuildsstuff Aug 20, 2020

njuCZ Aug 21, 2020

tombuildsstuff Aug 20, 2020

njuCZ Aug 21, 2020

tombuildsstuff Aug 20, 2020

njuCZ Aug 21, 2020

njuCZ commented Aug 24, 2020

jackofallops commented Sep 10, 2020

katbyte left a comment

njuCZ commented Sep 11, 2020

ghost commented Sep 17, 2020

ghost commented Oct 11, 2020


		* `create_mode` - (Optional) Specifies how to create the Sql Pool. Valid values are: `Default`, `Recovery` or `PointInTimeRestore`. Must be `Default` to create a new database. Defaults to `Default`.

		* `collation` - (Optional) The name of the collation to use with this pool. Applies only if `create_mode` is `Default`. Azure default is `SQL_LATIN1_GENERAL_CP1_CI_AS`. Changing this forces a new resource to be created.

new resource azurerm_synapse_sql_pool #8095

new resource azurerm_synapse_sql_pool #8095

Conversation

njuCZ commented Aug 12, 2020 • edited by katbyte Loading

katbyte left a comment

Choose a reason for hiding this comment

njuCZ commented Aug 13, 2020

WodansSon left a comment

Choose a reason for hiding this comment

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff Aug 20, 2020

Choose a reason for hiding this comment

njuCZ Aug 21, 2020

Choose a reason for hiding this comment

tombuildsstuff Aug 20, 2020

Choose a reason for hiding this comment

njuCZ Aug 21, 2020

Choose a reason for hiding this comment

tombuildsstuff Aug 20, 2020

Choose a reason for hiding this comment

njuCZ Aug 21, 2020

Choose a reason for hiding this comment

njuCZ commented Aug 24, 2020

jackofallops commented Sep 10, 2020

katbyte left a comment

Choose a reason for hiding this comment

njuCZ commented Sep 11, 2020

ghost commented Sep 17, 2020

ghost commented Oct 11, 2020

new resource `azurerm_synapse_sql_pool` #8095

new resource `azurerm_synapse_sql_pool` #8095

njuCZ commented Aug 12, 2020 •

edited by katbyte

Loading