-
Notifications
You must be signed in to change notification settings - Fork 427
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
feat: add on_account to session and object params #1685
Conversation
Integration tests failure for 8a178eeaf8dffacfc103f62e88acaeb5c455c3eb |
Integration tests failure for 3cff2eb3def90f33a840e78d8fd45c0223ce38b1 |
Integration tests success for a0b47aff7f01ef03a603f4a5762075d29d23dcd7 |
pkg/snowflake/parameters.go
Outdated
// prepared statements do not work here for some reason. We already validate inputs so its okay | ||
stmt := fmt.Sprintf("ALTER ACCOUNT SET %s = %s", v.key, v.value) | ||
_, err := v.db.Exec(stmt) | ||
return err |
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.
Could we reuse AccountParameterBuilder.SetParameter
to avoid duplicating this SQL? I assume these two use cases are functionally identical.
pkg/snowflake/parameters.go
Outdated
// prepared statements do not work here for some reason. We already validate inputs so its okay | ||
stmt := fmt.Sprintf("ALTER ACCOUNT SET %s = %s", v.key, v.value) | ||
_, err := v.db.Exec(stmt) | ||
return err |
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.
Same
@@ -143,9 +154,12 @@ func CreateObjectParameter(d *schema.ResourceData, meta interface{}) error { | |||
func ReadObjectParameter(d *schema.ResourceData, meta interface{}) error { | |||
db := meta.(*sql.DB) | |||
id := d.Id() | |||
parts := strings.Split(id, "❄️") | |||
parts := strings.Split(id, "|") |
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.
Maybe worth extracting "|" into a global variable to avoid magic values.
@@ -15,7 +16,7 @@ var parametersSchema = map[string]*schema.Schema{ | |||
"parameter_type": { | |||
Type: schema.TypeString, | |||
Optional: true, | |||
Default: "SESSION", | |||
Default: "ACCOUNT", |
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.
Is this backward compatible?
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.
the old behavior "SESSION" was actually the same as "ACCOUNT", hence setting it as "ACCOUNT" is the most sensible. This is generally what people want to do anyways
Integration tests failure for ae21f4dc832cb1f919b807bc91a39a28c356d6dc |
Integration tests success for 3449e6a0bd14e5ec0d928db9d1830c7db29d64a1 |
Allows session params to be created either on account or on user. And object params to be created either on an object, or on account.
issues #1565 , #1546