[feature] tasks

[henriblancke]

Attempt to add task support.

Couple of notes:

• There is no support for the copy grants option on tasks as we're never cloning or replacing an existing task
• after only supports tasks that are in the same schema (as per the docs)
• This PR doesn't implement task grants but we could make it part of it?
• The task is automatically started after creation

Travis build

[ryanking]

One thing I forgot on the previous review - can you add an acceptance test for this resource? Ideally one that exercises some of the update logic.

[ryanking]

@henriblancke it appears there is another PR covering this -> #198

Can you all work together on this?

[chriskuchin]

@henriblancke @ryanking Happy to collaborate here the core of our 2 MRs seems very similar minus a few cosmetic differences. The biggest difference seems to be in mine I handle temporarily suspending the root task of a tree so that the child tasks can be altered and then resume it after the fact.

From the docs:

Modifying a standalone task requires that the task is suspended.

Modifying any task in a tree of tasks requires that the root task is suspended.

Another difference is that in my PR i directly expose the state (Suspended|Started) via an enabled flag.

Anyways let me know how I can Help.

[henriblancke]

@chriskuchin yeah your handling of task dependencies seems much better. Having the option to enable/disable the task is also nice! One thing I noticed tho is that you're not exposing session_parameters in your PR? Maybe we should just add that and we're good to go? It's up to @ryanking on how you all want to handle this :)

[chriskuchin]

Yeah I got distracted and forgot about them. Happy to put a little more time into the PR if that's what we wanna do and add that

[ryanking]

One thing I noticed tho is that you're not exposing session_parameters in your PR?

Perhaps we can move forward by merging @chriskuchin's PR then adding session_params as a separate PR?

[chriskuchin]

I nearly have the session_parameters piece done on my PR just testing and debugging now @ryanking

[henriblancke]

@ryanking @chriskuchin I actually went ahead and added the enabled flag and logic to suspend root or standalone tasks before modifying / creating a child task. I think my use of defer to reactivate the suspended root task might be a little cleaner and safer when anything else fails during modifying or creating a new task. That way we'll always re-enable the root task? Let me know what you all think...

[chriskuchin]

@henriblancke sounds good. Have you tested you session parameters? I had to modify them pretty heavily for them to actually work. Plus you weren't reading their state at all.

[henriblancke]

@chriskuchin it did test em, but maybe not enough 😄? Can you elaborate? What do you mean by not reading state? Thanks!

[chriskuchin]

You never read at least that I could find the parameters after you set them. The other problem I had was you were escaping the parameter values which caused them to be invalid as well as not wrapping them in quotes

[chriskuchin]

They aren't even running they error out about not being configured in Travis. Are they passing for you locally?

[chriskuchin]

Ahh you have the acceptance tests hooked up to your fork... I have no idea that was never an error I saw when writing them or getting things working. Sounds like it's an error in the first step. You can enable tf_logs=debug to get more info but it's really noisy so might be hard. Can you run it locally so you run just that test with debug?

[chriskuchin]

So it seems to have something to do with your state. The error occurs during a state refresh so it's probably in the read function I would guess

[henriblancke]

@chriskuchin yeah just figured it out adding the id we get from snowflake during reading the task caused the error. It breaks taskIDFromString. I'm running into more problems tho now that this one is resolved

[chriskuchin]

ahh that makes sense I didn't follow that compound id pattern in My MR 👍 let me know if i can help

[ryanking]

@ryanking not sure why the Acceptance tests aren't running in your CI but as long as those are passing the PR looks good to me

ok i will take a look

[henriblancke]

@ryanking they should be all green here

[ryanking]

This looks quite good, most of my comments here are matters of style.

[ryanking]

Suggested change

	for {
	builder := snowflake.Task(name, database, dbSchema)
	for {
	builder := snowflake.Task(name, database, dbSchema)

[ryanking]

This conditional is not very clear. Why do we ignore errors when name == data.Get("name")?