feat: add app packages #2323
feat: add app packages #2323
Conversation
|
Integration tests failure for c38f951c7e00ba613d045d82880402154baef6d4 |
pkg/sdk/application_packages_def.go
Outdated
| OptionalSetTags(). | ||
| OptionalUnsetTags(). | ||
| WithValidation(g.ValidIdentifier, "name"). | ||
| WithValidation(g.ExactlyOneValueSet, "Set", "UnsetDataRetentionTimeInDays", "UnsetMaxDataExtensionTimeInDays", "UnsetDefaultDdlCollation", "UnsetComment", "UnsetDistribution", "ModifyReleaseDirective", "SetDefaultReleaseDirective", "SetReleaseDirective", "UnsetReleaseDirective", "AddVersion", "DropVersion", "AddPatchForVersion", "SetTags", "UnsetTags"), |
There was a problem hiding this comment.
why you treat set and unset differently (I mean these 5: "UnsetDataRetentionTimeInDays", "UnsetMaxDataExtensionTimeInDays", "UnsetDefaultDdlCollation", "UnsetComment", "UnsetDistribution")? And why we are limiting the unset query to just one unset per statement? According to the docs we can unset multiple parameters as one statement. This is also what we are doing for other resources.
There was a problem hiding this comment.
I tried, set multiple params works, but unset multiple params does not work
There was a problem hiding this comment.
It works according to the docs: https://docs.snowflake.com/en/sql-reference/sql/alter-application-package#syntax. I checked it with a sample code:
-- DROP APPLICATION PACKAGE "aswtesting17012024";
CREATE APPLICATION PACKAGE "aswtesting17012024";
ALTER APPLICATION PACKAGE "aswtesting17012024" SET COMMENT = 'some comment' DISTRIBUTION = INTERNAL;
ALTER APPLICATION PACKAGE "aswtesting17012024" UNSET COMMENT, DISTRIBUTION;
I guess that the comma is missing for the UNSET in the docs but it seems to work just fine.
There was a problem hiding this comment.
created an unset helper struct
There was a problem hiding this comment.
But still, you limited it to just one field that can be unset, while according to the SQL statements I have attached above you can do multiple unset.
It should be configured similarly to e.g. storage integration.
So in your case please change:
OptionalQueryStructField(
"Unset",
applicationPackageUnset,
g.KeywordOptions().SQL("UNSET"),
).
to
OptionalQueryStructField(
"Unset",
applicationPackageUnset,
g.ListOptions().NoParentheses().SQL("UNSET"),
).
and
validation type in applicationPackageUnset from ExactlyOneValueSet to AtLeastOneValueSet.
We can merge this one and you will correct it in the follow-up PR. WDYT?
There was a problem hiding this comment.
sure, i will fix in a follow up PR
|
Integration tests failure for 89d07384390b6ec16feb9fb0e28d74ddb877b589 |
|
Integration tests failure for 15613015edbdd9cdb184a9f5e6d2492e17248dea |
|
Integration tests failure for 98ec7bb00d32e547e8347275b0a0ff4e29e644be |
Follow up PR from discussion in original PR #2323 (comment) Fixing Unset to use commas, allowing multiple unsets
https://docs.snowflake.com/en/sql-reference/sql/create-application-package
adds app packages to snowflake sdk