-
Notifications
You must be signed in to change notification settings - Fork 416
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: Task resource v1 readiness #3113
base: main
Are you sure you want to change the base?
Conversation
Integration tests failure for 28bf6a5cb6a95fa2d9cb42a558710bb4ad08ee58 |
Integration tests failure for 62adccb771be11faeea226dd4b4b4c29fcea2833 |
func (t *TaskResourceAssert) HasAfterIds(ids ...sdk.SchemaObjectIdentifier) *TaskResourceAssert { | ||
t.AddAssertion(assert.ValueSet("after.#", strconv.FormatInt(int64(len(ids)), 10))) | ||
for i, id := range ids { | ||
t.AddAssertion(assert.ValueSet(fmt.Sprintf("after.%d", i), id.FullyQualifiedName())) |
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 order shouldn't matter here.
|
||
import ( | ||
"fmt" | ||
"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk" |
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.
reviewdog
return u. | ||
HasSuspendTaskAfterNumFailures(10). | ||
HasTaskAutoRetryAttempts(0). | ||
HasUserTaskManagedInitialWarehouseSize("Medium"). |
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.
Use const from sdk.
HasClientSessionKeepAliveHeartbeatFrequency(3600). | ||
HasClientTimestampTypeMapping(sdk.ClientTimestampTypeMappingLtz). | ||
HasDateInputFormat("AUTO"). | ||
HasDateOutputFormat("YYYY-MM-DD"). |
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.
Hmm, I wonder if Snowflake has a list of available fields, and if we could use enums here if it makes sense (in other places as well).
HasTransactionAbortOnError(false). | ||
HasTransactionDefaultIsolationLevel(sdk.TransactionDefaultIsolationLevelReadCommitted). | ||
HasTwoDigitCenturyStart(1970). | ||
HasUnsupportedDdlAction(sdk.UnsupportedDDLAction(strings.ToLower(string(sdk.UnsupportedDDLActionIgnore)))). |
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.
Just curious, why do we convert to lower case?
HasSuspendTaskAfterNumFailuresString("2"), | ||
resourceshowoutputassert.TaskShowOutput(t, rootTaskModel.ResourceReference()). | ||
HasSchedule(schedule). | ||
// HasTaskRelations(sdk.TaskRelations{FinalizerTask: &finalizerTaskId}). |
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.
What about these comments?
resourceassert.TaskResource(t, firstTaskStandaloneModelDisabled.ResourceReference()). | ||
HasScheduleString(schedule). | ||
HasEnabledString(r.BooleanFalse). | ||
HasSuspendTaskAfterNumFailuresString("10"), |
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.
We should also check that finalized/finalizer fields are empty.
basicConfigModel := model.TaskWithId("test", id, false, statement) | ||
|
||
// New warehouse created, because the common one has lower-case letters that won't work | ||
warehouse, warehouseCleanup := acc.TestClient().Warehouse.CreateWarehouse(t) |
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.
We already have SNOW-1541938 for streamlits, we should create a similar ticket for this.
expectedTaskConfig := strings.ReplaceAll(taskConfig, "$", "") | ||
taskConfigVariableValue := "$" + taskConfig | ||
comment := random.Comment() | ||
condition := `SYSTEM$STREAM_HAS_DATA('MYSTREAM')` |
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.
I think we should have some test cases for different spaces and new lines for fields with SQL statements.
HasConfig(expectedTaskConfig). | ||
HasBudget(""). | ||
HasTaskRelations(sdk.TaskRelations{}), | ||
), |
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.
Pls add a test case with external changes.
Changes
Next prs
References