feat: Task resource v1 readiness #3113

sfc-gh-jcieslak · 2024-10-03T14:57:12Z

Changes

Mostly done task resource refactor
Sweepers test fixed (they always added errors even when they're nil)
Minor changes in SDK (find out the bug with alter task and right now we're only supporting upper-cased warehouse names; added that to the field description)
Acceptance tests
The enabled field is not a three-valued boolean because it created certain issues when updating the task. The task status is very important for their management and working, so I decided to have them as regular boolean values that are either true or false, nothing in between.

Next prs

Apply comments from feat: Tasks v1 readiness - SDK part #3086
Data source
Move some of the logic to SDK
Refactor SDK suspend root logic for tasks (and overall suspend logic in SDK/resource)
Examples, documentation, and migration guide
More test cases for complicated DAG structures to show the resource can handle more complex structures
Analyze non-deterministic test cases
Refactor task resuming in task resource (most likely with the use of defer) because currently, there may be cases that error can cause tasks to be not resumed.
Test and see how the DAG of tasks could be owned by a role other than the one created with Terraform (also with less privileges, only to run the task).
Make config without $$ escapes needed
Check if more helper functions from user resource could be used in task resource.
Check in one acceptance test why finalizer task in show_output is not set (is that Snowflake or mapping error).
More tests for calling (as field) - feat: Task resource v1 readiness #3113 (comment)
Re-generate and list all the issues with asserts and models
check test tasks_gen_integration_test.go:937 (and see why it's non deterministic).
Support session paramters

References

CREATE TASK

github-actions · 2024-10-03T16:59:34Z

Integration tests failure for 28bf6a5cb6a95fa2d9cb42a558710bb4ad08ee58

github-actions · 2024-10-04T11:04:05Z

Integration tests failure for 62adccb771be11faeea226dd4b4b4c29fcea2833

sfc-gh-jmichalak · 2024-10-07T08:09:38Z

pkg/acceptance/bettertestspoc/assert/resourceassert/task_resource_ext.go

+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()))


The order shouldn't matter here.

SHouldn't because the assertion does not have InOrder in name or because Snowflake ordering for alter in non-deterministic?

Yeah, but that should be resolved in newly added TODO for asserts to generate or create a helper for creating asserts for list with non-deterministic order. Right now, this one work (despite being reliant on order) and I would leave it as is.

Let's at least add InOrder suffix to the name of the function then (may be added in the next PR)

I think we discussed this in one of the previous PRs. Snowflake does not guarantee item order, so in general we should compare ID collections, not one by one. For cases where the order matters, we should add a comment about that.

Addressed in: For now, renamed to have InOrder suffix, the limitation of order was added to assert improvements some time ago (it should be in main or in this pr.)

pkg/acceptance/bettertestspoc/assert/resourceassert/task_resource_ext.go

pkg/acceptance/bettertestspoc/assert/resourceparametersassert/task_resource_parameters_ext.go

sfc-gh-jmichalak · 2024-10-07T09:48:16Z

pkg/resources/task_acceptance_test.go

+						HasSuspendTaskAfterNumFailuresString("2"),
+					resourceshowoutputassert.TaskShowOutput(t, rootTaskModel.ResourceReference()).
+						HasSchedule(schedule).
+						// HasTaskRelations(sdk.TaskRelations{FinalizerTask: &finalizerTaskId}).


What about these comments?

Hmm missed them somehow. Uncommented + added asserts to the rest of the steps.

I left TODO for the next pr to analyze this more in-depth. For now, I don't see why it's not there. I'll take a look if that's a mapping or Snowflake issue.

After a short analysis, It seems like a bug in Snowflake. For now, I removed this assert and added an entry in our doc about Snowflake improvements. I'll fill it later.

pkg/resources/task_acceptance_test.go

sfc-gh-jmichalak · 2024-10-07T09:53:03Z

pkg/resources/task_acceptance_test.go

+	expectedTaskConfig := strings.ReplaceAll(taskConfig, "$", "")
+	taskConfigVariableValue := "$" + taskConfig
+	comment := random.Comment()
+	condition := `SYSTEM$STREAM_HAS_DATA('MYSTREAM')`


I think we should have some test cases for different spaces and new lines for fields with SQL statements.

True, in upcoming tests I also wanted to add tests for different kinds of things in as body. I'll link this comment in the desc and apply when testing other as options.

sfc-gh-jmichalak · 2024-10-07T09:55:03Z

pkg/resources/task_acceptance_test.go

+						HasConfig(expectedTaskConfig).
+						HasBudget("").
+						HasTaskRelations(sdk.TaskRelations{}),
+				),


Pls add a test case with external changes.

Added test cases for Finalize and After fields, added TODO for next pr to see what also could be tested

pkg/acceptance/bettertestspoc/assert/objectassert/task_snowflake_gen.go

pkg/acceptance/bettertestspoc/assert/objectparametersassert/task_parameters_snowflake_gen.go

sfc-gh-asawicki · 2024-10-09T09:27:31Z

pkg/acceptance/bettertestspoc/assert/resourceassert/task_resource_ext.go

+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()))


SHouldn't because the assertion does not have InOrder in name or because Snowflake ordering for alter in non-deterministic?

pkg/acceptance/bettertestspoc/assert/resourceparametersassert/task_resource_parameters_ext.go

pkg/internal/collections/collection_helpers_test.go

pkg/schemas/task_gen.go

pkg/resources/task.go

pkg/resources/task_acceptance_test.go

sfc-gh-asawicki · 2024-10-09T09:55:52Z

pkg/resources/task_acceptance_test.go

+						HasConfig(expectedTaskConfig).
+						HasBudget("").
+						HasTaskRelations(sdk.TaskRelations{}),
+				),


# Conflicts: # pkg/acceptance/bettertestspoc/README.md # pkg/acceptance/bettertestspoc/assert/resourceassert/gen/resource_schema_def.go # pkg/sdk/sweepers_test.go

github-actions · 2024-10-14T16:01:44Z

Integration tests failure for 80ca12e07e4e1b0fa5a0d6a8245c017496fa00cd

github-actions · 2024-10-14T16:21:58Z

Integration tests failure for f0514bcb9d530e9f9afcb89bac43b56a59d69254

github-actions · 2024-10-14T16:32:24Z

Integration tests failure for ee9583dfa93da2a748b9003843edc1fa844742f4

sfc-gh-asawicki · 2024-10-16T08:01:32Z

pkg/schemas/task_gen.go

@@ -147,6 +145,7 @@ func TaskToSchema(task *sdk.Task) map[string]any {
 	taskSchema["config"] = task.Config
 	taskSchema["budget"] = task.Budget
 	taskSchema["last_suspended_reason"] = task.LastSuspendedReason
+	// This is manually edited, please don't re-generate this file


nit (next PR): please keep this changes after regenerating the file

# Conflicts: # pkg/acceptance/bettertestspoc/README.md # pkg/acceptance/bettertestspoc/assert/resourceassert/gen/resource_schema_def.go

github-actions · 2024-11-04T11:25:06Z

Integration tests failure for 012fb1835c3fceb166f9b81f159e7e09a9af61de

github-actions · 2024-11-04T14:00:57Z

Integration tests failure for 90d70469b3b6fadb123d943875b92dac69d6c261

- [x] Apply comments from #3086 - [x] Check if more helper functions from user resource could be used in task resource. - [ ] Apply comments from #3113 - [x] Cron #3113 (comment) - [ ] Resource logic #3113 (comment) - [ ] Refactor SDK suspend root logic for tasks (and overall suspend logic in SDK/resource) #3113 (comment) - [ ] Move some of the logic to SDK (if possible) - [ ] #3170 (comment) - [ ] Refactor task resuming in task resource (most likely with the use of defer) because currently, there may be cases that error can cause tasks to be not resumed. - [ ] Tests - [ ] External changes - #3113 (comment) - [ ] Add more complicated DAG structures to show the resource can handle more complex structures - [ ] Calling (`as` field) - #3113 (comment) - [ ] For showing how the DAG of tasks could be owned by a role other than the one created with Terraform (also with less privileges, only to run the task). - [ ] Check in one acceptance test why finalizer task in show_output is not set (is that Snowflake or mapping error). - [ ] Data source - [ ] Examples, documentation, and migration guide - [ ] Keep manually changed files after regeneration #3113 (comment) - [ ] Make config without $$ escapes needed - [ ] Support session paramters - [ ] Analyze non-deterministic test cases - [ ] Check test tasks_gen_integration_test.go:937 (and see why it's non deterministic). - [ ] Re-generate and list all the issues with asserts and models

## Changes - Fixed the need to enclose `config` with $$ - Added full datasource support with tests - Documentation and examples (v1 candidates; added for resource and datasource) - Migration guide (now only for datasource) - Extracted schema for regular `in` filtering - Describe wasn't added on purpose, because it seems to mirror values from what we get from the SHOW command (can also add it, but for now it doesn't provide any benefits) ## List from previous prs - [ ] Apply comments #3170 - [ ] Apply comments from #3113 - [x] Cron #3113 (comment) - [ ] Resource logic #3113 (comment) - [ ] Refactor SDK suspend root logic for tasks (and overall suspend logic in SDK/resource) #3113 (comment) - [ ] Move some of the logic to SDK (if possible) - [ ] #3170 (comment) - [ ] Refactor task resuming in task resource (most likely with the use of defer) because currently, there may be cases that error can cause tasks to be not resumed. - [ ] Tests - [ ] External changes - #3113 (comment) - [ ] Add more complicated DAG structures to show the resource can handle more complex structures - [ ] Calling (`as` field) - #3113 (comment) - [ ] For showing how the DAG of tasks could be owned by a role other than the one created with Terraform (also with less privileges, only to run the task). - [x] Check in one acceptance test why finalizer task in show_output is not set (is that Snowflake or mapping error). - [x] Data source - [ ] Examples, documentation, and migration guide - [ ] Keep manually changed files after regeneration #3113 (comment) - [x] Make config without $$ escapes needed - [ ] Support session paramters - [ ] Analyze non-deterministic test cases - [ ] Check test tasks_gen_integration_test.go:937 (and see why it's non deterministic). - [ ] Re-generate and list all the issues with asserts and models

- [x] Apply comments from #3113 - [x] Cron #3113 (comment) - [x] Resource logic #3113 (comment) - [x] Refactor SDK suspend root logic for tasks (and overall suspend logic in SDK/resource) #3113 (comment) - [x] Tests - [x] External changes - #3113 (comment) - [x] Add more complicated DAG structures to show the resource can handle more complex structures - [x] Calling (`as` field) - #3113 (comment) - [x] Check in one acceptance test why the finalizer task in show_output is not set (is that Snowflake or mapping error). - [x] Data source - [x] Examples, documentation, and migration guide - [x] Keep manually changed files after regeneration #3113 (comment) - [x] Make config without $$ escapes needed - [x] Support session parameters (I think it's already done, but check https://docs.snowflake.com/en/sql-reference/parameters#label-session-parameters) - [x] Refactor task resuming in task resource (most likely with the use of defer) because currently, there may be cases that error can cause tasks to be not resumed. - [x] Analyze non-deterministic test cases (All tests seem to be deterministic, only object_parameter resource tests are causing the AllParameters test to fail) - [x] Check test tasks_gen_integration_test.go:937 (and see why it's non-deterministic). - [x] Move some of the logic to SDK (if possible) - [x] #3170 (comment) - [x] Apply comments #3170 - [ ] Re-generate and list all the issues with asserts and models

Pr that collects all the changes to tasks from previous prs: - #3202 - #3170 - #3113 **Note**: look at the last commit, added very small changes to documentation and code (around 8 lines)

sfc-gh-jcieslak added 6 commits September 30, 2024 11:02

wip

38b8550

wip

6837bc5

wip

c2f707d

wip

6843e03

wip

12eadb0

wip

28bf6a5

wip

62adccb

sfc-gh-jcieslak marked this pull request as ready for review October 4, 2024 09:20

sfc-gh-jmichalak requested changes Oct 7, 2024

View reviewed changes

sfc-gh-asawicki requested changes Oct 9, 2024

View reviewed changes

sfc-gh-jcieslak added 4 commits October 11, 2024 16:44

Merge branch 'refs/heads/main' into task-resource-v1-readiness

bba9569

# Conflicts: # pkg/acceptance/bettertestspoc/README.md # pkg/acceptance/bettertestspoc/assert/resourceassert/gen/resource_schema_def.go # pkg/sdk/sweepers_test.go

wip

80ca12e

docs re-generated

f0514bc

Added test that proves the issue

ee9583d

sfc-gh-asawicki self-requested a review October 16, 2024 07:43

sfc-gh-asawicki previously approved these changes Oct 16, 2024

View reviewed changes

sfc-gh-jmichalak previously approved these changes Oct 16, 2024

View reviewed changes

sfc-gh-jcieslak added 2 commits November 4, 2024 11:37

Merge branch 'refs/heads/main' into task-resource-v1-readiness

90404b9

# Conflicts: # pkg/acceptance/bettertestspoc/README.md # pkg/acceptance/bettertestspoc/assert/resourceassert/gen/resource_schema_def.go

Resolve merge conflicts

012fb18

sfc-gh-jcieslak dismissed stale reviews from sfc-gh-jmichalak and sfc-gh-asawicki via 012fb18 November 4, 2024 11:22

Resolve merge conflicts and fix failing tests

90d7046

sfc-gh-jmichalak approved these changes Nov 4, 2024

View reviewed changes

sfc-gh-jcieslak mentioned this pull request Nov 4, 2024

feat: Task resource v1 readiness part 2 #3170

Merged

23 tasks

sfc-gh-asawicki approved these changes Nov 5, 2024

View reviewed changes

sfc-gh-jcieslak changed the base branch from main to tasks-v1-readiness November 5, 2024 10:25

sfc-gh-jcieslak merged commit 0aba5c9 into tasks-v1-readiness Nov 5, 2024
10 of 11 checks passed

sfc-gh-jcieslak deleted the task-resource-v1-readiness branch November 5, 2024 13:33

This was referenced Nov 14, 2024

feat: Task datasource and fixes #3195

Merged

feat: Tasks v1 readiness part3 #3202

Merged

sfc-gh-jcieslak mentioned this pull request Nov 25, 2024

feat: Tasks v1 readiness #3222

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Task resource v1 readiness #3113

feat: Task resource v1 readiness #3113

sfc-gh-jcieslak commented Oct 3, 2024 •

edited

Loading

github-actions bot commented Oct 3, 2024

github-actions bot commented Oct 4, 2024

sfc-gh-jmichalak Oct 7, 2024

sfc-gh-asawicki Oct 9, 2024

sfc-gh-jcieslak Oct 11, 2024

sfc-gh-asawicki Oct 16, 2024

sfc-gh-jmichalak Oct 16, 2024

sfc-gh-jcieslak Nov 4, 2024 •

edited

Loading

sfc-gh-jmichalak Oct 7, 2024

sfc-gh-jcieslak Oct 14, 2024

sfc-gh-jcieslak Oct 14, 2024

sfc-gh-jcieslak Nov 14, 2024

sfc-gh-jmichalak Oct 7, 2024

sfc-gh-jcieslak Oct 14, 2024

sfc-gh-jmichalak Oct 7, 2024

sfc-gh-asawicki Oct 9, 2024

sfc-gh-jcieslak Oct 14, 2024

sfc-gh-asawicki Oct 9, 2024

sfc-gh-asawicki Oct 9, 2024

github-actions bot commented Oct 14, 2024

github-actions bot commented Oct 14, 2024

github-actions bot commented Oct 14, 2024

sfc-gh-asawicki Oct 16, 2024

github-actions bot commented Nov 4, 2024

github-actions bot commented Nov 4, 2024

feat: Task resource v1 readiness #3113

feat: Task resource v1 readiness #3113

Conversation

sfc-gh-jcieslak commented Oct 3, 2024 • edited Loading

Changes

Next prs

References

github-actions bot commented Oct 3, 2024

github-actions bot commented Oct 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-jcieslak Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 14, 2024

github-actions bot commented Oct 14, 2024

github-actions bot commented Oct 14, 2024

Choose a reason for hiding this comment

github-actions bot commented Nov 4, 2024

github-actions bot commented Nov 4, 2024

sfc-gh-jcieslak commented Oct 3, 2024 •

edited

Loading

sfc-gh-jcieslak Nov 4, 2024 •

edited

Loading