Skip to content
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 part 2 #3170

Merged
merged 23 commits into from
Nov 14, 2024

Conversation

sfc-gh-jcieslak
Copy link
Collaborator

@sfc-gh-jcieslak sfc-gh-jcieslak commented Nov 4, 2024

# Conflicts:
#	pkg/acceptance/bettertestspoc/README.md
#	pkg/acceptance/bettertestspoc/assert/resourceassert/gen/resource_schema_def.go
#	pkg/sdk/sweepers_test.go
# Conflicts:
#	pkg/acceptance/bettertestspoc/README.md
#	pkg/acceptance/bettertestspoc/assert/resourceassert/gen/resource_schema_def.go
@sfc-gh-jcieslak sfc-gh-jcieslak changed the title wip feat: task resource v1 readiness part 2 Nov 4, 2024
@sfc-gh-jcieslak sfc-gh-jcieslak changed the title feat: task resource v1 readiness part 2 feat: Task resource v1 readiness part 2 Nov 4, 2024
Copy link

github-actions bot commented Nov 4, 2024

Integration tests failure for fdb41907fe23e809a79541869896bfd2cd7c3d4a

Base automatically changed from task-resource-v1-readiness to tasks-v1-readiness November 5, 2024 13:33
…adiness-part2

# Conflicts:
#	pkg/acceptance/bettertestspoc/assert/resourceassert/task_resource_ext.go
#	pkg/acceptance/bettertestspoc/assert/resourceassert/task_resource_gen.go
#	pkg/acceptance/bettertestspoc/assert/resourceshowoutputassert/task_show_output_ext.go
#	pkg/acceptance/bettertestspoc/assert/resourceshowoutputassert/task_show_output_gen.go
#	pkg/acceptance/bettertestspoc/config/model/task_model_ext.go
#	pkg/acceptance/bettertestspoc/config/model/task_model_gen.go
#	pkg/resources/task.go
#	pkg/resources/task_acceptance_test.go
#	pkg/resources/task_parameters.go
#	pkg/sdk/testint/tasks_gen_integration_test.go
@sfc-gh-jcieslak sfc-gh-jcieslak force-pushed the task-resource-v1-readiness-part2 branch from fdb4190 to 62edcec Compare November 7, 2024 15:59
Copy link

github-actions bot commented Nov 7, 2024

Integration tests failure for 62edcec8ba33b2f58fc3ef67c0e01e1c29f36898

Copy link

github-actions bot commented Nov 8, 2024

Integration tests failure for a90fb21699fa6518baa22a729635c509469c0b56

@sfc-gh-jcieslak sfc-gh-jcieslak marked this pull request as ready for review November 8, 2024 13:21
Copy link

github-actions bot commented Nov 8, 2024

Integration tests failure for b73d55ac3062a28ef71222f98d151a05fbed6333

@sfc-gh-asawicki sfc-gh-asawicki self-requested a review November 11, 2024 21:14
docs/resources/task.md Outdated Show resolved Hide resolved
pkg/resources/task.go Show resolved Hide resolved
pkg/resources/task.go Outdated Show resolved Hide resolved
pkg/resources/task.go Show resolved Hide resolved
pkg/resources/task.go Outdated Show resolved Hide resolved
pkg/resources/resource_helpers_read.go Outdated Show resolved Hide resolved
func() error {
upperSchedule := strings.ToUpper(task.Schedule)
switch {
case strings.Contains(upperSchedule, "USING CRON"):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this logic is really close to the SDK, let's move it maybe (in the next PR), we can discuss how in the next PR

@@ -62,6 +62,17 @@ func attributeDirectValueCreate[T any](d *schema.ResourceData, key string, creat
return nil
}

func attributeMappedValueCreate[T any](d *schema.ResourceData, key string, createField **T, mapper func(value any) (*T, error)) error {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: we can call this in other functions above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed them, but it seemed to complicate the code (it's easier to understand the above functions the way they are right now). I would leave them as they are right now.

pkg/resources/resource_helpers_read.go Outdated Show resolved Hide resolved
pkg/resources/task.go Outdated Show resolved Hide resolved
pkg/resources/task.go Outdated Show resolved Hide resolved

if newSchedule != nil && len(newSchedule.([]any)) == 1 {
if _, newMinutes := d.GetChange("schedule.0.minutes"); newMinutes.(int) > 0 {
set.Schedule = sdk.String(fmt.Sprintf("%d MINUTE", newMinutes.(int)))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: such functions for setting create/alter fields could be added to the sdk.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but I would leave it as a future improvement. I made it this way because we already have a similar approach to alerts.

Copy link

Integration tests failure for a200cfeb31e2744690bb37294a5cfa5295ccc218

Copy link

Integration tests failure for ae9c5c3e770eaafc93070d85f1e745e318f2b80c

@sfc-gh-asawicki sfc-gh-asawicki self-requested a review November 13, 2024 15:35
### snowflake_task resource changes
Changes:
- `enabled` field changed to `started` and type changed to string with only boolean values available (see ["empty" values](./v1-preparations/CHANGES_BEFORE_V1.md#empty-values))
- `shedule` field changed from single value to nested object that allows for specifying either minutes or cron
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit (maybe for next PRs): instructions for user what they need to do and what will be handled automatically

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, probably next pr, because there's no state upgrader yet

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, a typo: schedule

pkg/resources/task.go Show resolved Hide resolved
Copy link

Integration tests failure for 209b04e74051903f1ef2841d39a3bdc5b75a5690

### snowflake_task resource changes
Changes:
- `enabled` field changed to `started` and type changed to string with only boolean values available (see ["empty" values](./v1-preparations/CHANGES_BEFORE_V1.md#empty-values))
- `shedule` field changed from single value to nested object that allows for specifying either minutes or cron
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, a typo: schedule

Copy link

Integration tests failure for 4e0088271a6d33166f46d39a86a281cc7be3df2c

@sfc-gh-asawicki sfc-gh-asawicki self-requested a review November 14, 2024 12:50
@sfc-gh-jcieslak sfc-gh-jcieslak merged commit af50770 into tasks-v1-readiness Nov 14, 2024
8 of 9 checks passed
@sfc-gh-jcieslak sfc-gh-jcieslak deleted the task-resource-v1-readiness-part2 branch November 14, 2024 13:00
sfc-gh-jcieslak added a commit that referenced this pull request Nov 18, 2024
## 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
sfc-gh-jcieslak added a commit that referenced this pull request Nov 25, 2024
- [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
sfc-gh-jcieslak added a commit that referenced this pull request Nov 26, 2024
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants