Skip to content
This repository has been archived by the owner on Nov 18, 2021. It is now read-only.

cmd/cue/cmd: control task dependency using before and after fields #200

Closed
wants to merge 1 commit into from

Conversation

adieu
Copy link
Contributor

@adieu adieu commented Nov 28, 2019

Currently we resolve task dependency by checking reference to incomplete task variable. We cannot do things like create a file first, then run a command with it.

For example this command would not work cause write and ansible will be executed at the same time.

command: play: {
	task: write: file.Create & {
		filename: "playbook.yml"
		contents:  yaml.MarshalStream([playbook])
	}
	task: ansible: exec.Run & {
		cmd:    "ansible-playbook playbook.yml"
		stdout: string
	}
	task: display: cli.Print & {
		text: task.ansible.stdout
	}
}

Using this PR, we could declare dependency and reverse dependency using before and after fields. The value could be either a task or a list of tasks. Now we can rewrite the command like this to make it work.

command: play: {
	task: write: file.Create & {
		filename: "playbook.yml"
		contents:  yaml.MarshalStream([playbook])
	}
	task: ansible: exec.Run & {
		cmd:    "ansible-playbook playbook.yml"
		stdout: string
		after: task.write
	}
	task: display: cli.Print & {
		text: task.ansible.stdout
	}
}

Copy link
Contributor

@mpvl mpvl left a comment

Choose a reason for hiding this comment

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

This seems reasonable, but requires some tests. A testdata/script test would be fine.

@adieu
Copy link
Contributor Author

adieu commented Dec 2, 2019

I'll add a test for this PR.

@adieu adieu force-pushed the task-dependency branch 2 times, most recently from a0a86fb to 17939e0 Compare December 3, 2019 16:43
stdout: string
}
t4: exec.Run & {
cmd: ["sh", "-c", "a=\(strings.TrimSpace(task.t1.stdout));b=\(strings.TrimSpace(task.t2.stdout));if [ $a -lt $b ]; then echo 'true'; fi"]
Copy link
Contributor

Choose a reason for hiding this comment

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

This fails here. The -lt option only supports integers. On MacOS, however, the %N option is not recognized, so MacOS adds an arbitrary N at the end of the number. It couldn't find a high-precision timer option for macOS' date. So maybe sleep for 2 seconds to be safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry. My last commit passed the test but I thought I could speed up the test a little bit using %N. Went to bed after pushing the code. Didn't wait for the test to finish.

I might also fixed another possible issue with t != dep check. I'll try add another test case too.

Copy link
Contributor Author

@adieu adieu Dec 4, 2019

Choose a reason for hiding this comment

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

I have to sleep for 2 seconds in t1 and 1 second in t2 to make the test case accurate. Without dependency injection, t1 and t2 would be started at the same time making t1.output > t2.output. With dependency injection, t2 will be started after t1 making t1.output < t2.output.

Removing the 1 second sleep in t2 will make t1.output == t2.output. So there was total 3 seconds of sleep.

Can we do a 2 seconds sleep in t1, removing the 1 second sleep in t2 and use -le for the check? I think it should work.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thinks so too.

@adieu
Copy link
Contributor Author

adieu commented Dec 4, 2019

@mpvl This fix 52b04d4#diff-104bcbf8410c0cd325212e1b0af431aeL90 will cause the Equals check of two list value to fail. Even when they are the same.

Removing the optionals check will let my test case pass. I'm not quite sure why we directly return false here.

@mpvl
Copy link
Contributor

mpvl commented Dec 4, 2019

@adieu Do you have an example of a specific equals check that fails?

}

// Inject reverse dependency in `before` field
before := task.Lookup("before")
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, the cue files in pkg/tools will have to be updated as well for documentation and client-side validation purposes. Possibly this can be done later.

Either way, this change will have to be added after the next release which is predominantly a bug fix only change. If that is okay with you.

There are some notable changes related to the tooling layer that will make it more flexible and future-proof and will incorporate flag parsing. Long story short, some of the future task nodes (like flags and envs) might having naming conflicts with before and after. A simple solution may be to name them $before and $after. This may not be so pretty. Another option may be to allow another option in task (staring with $) to disable before and after for this node. We'll figure something out either way.

@mpvl
Copy link
Contributor

mpvl commented Dec 4, 2019

@adieu

@adieu Do you have an example of a specific equals check that fails?

Oh, I see you already did, and very properly at it. :)
I see why this is failing, but not adding the optional check breaks other things. I will just have to make subsumption more accurate there.

Note that equality checks for structs are a somewhat dubious notion, though. This is one reason why == is not defined for structs. It should be documented in Equals that equality doesn't always yield accurate results for incomplete composite structures. This is not special to CUE, but to many logical programming languages.

@mpvl
Copy link
Contributor

mpvl commented Dec 4, 2019

Anyway, at the very least it should return true for structural equality.

mpvl added a commit that referenced this pull request Dec 5, 2019
Equal is only defined for for concrete values, so it
makes sense to ignore optional fields.

Issue #200
Issue #207

Change-Id: I96e9fc36a986cc9e95a412cdab4409a600eeab84
Reviewed-on: https://cue-review.googlesource.com/c/cue/+/4300
Reviewed-by: Marcel van Lohuizen <mpvl@golang.org>
@adieu
Copy link
Contributor Author

adieu commented Dec 6, 2019

I've rebased to master using $before and $after. The tests passed.

I found the declarative scripting feature very powerful when I was hacking on the code and thinking about its possible usage patterns. Right now it's just at a very early stage and there are quite a few things we could make improvements to. I made this PR to get https://github.com/adieu/cue-ansible/blob/master/play_tool.cue working so that I could demonstrate it to other people the power of CUE.

How about we get this PR in and discuss the tooling layer design in another issue or slack?

@mpvl
Copy link
Contributor

mpvl commented Dec 6, 2019

Yeah, the next release should see some big improvements.

By making flags and envs tasks, I don't need any top-level fields anymore. This means that the task struct can go. Instead one would have an arbitrary hierarchy of tasks. This makes one-task commands much easier to specify (removes two levels of hierarchy). And it makes larger tasks more composable.

Then in the next phase of the testing command, I would like add support for testing tools. The data-driven scripting makes it very easy to do injection from within the language itself (just unify the command graph with the test data, and all tasks whose inputs are now concrete can run). It could even be constraint to only run the tasks in the expected result set, and their dependencies, as others would be immaterial. Then a next step could even be to automatically generate test sets from manual runs (especially handy when this involves http calls etc.). Again, it is just data. Anyway, getting ahead of myself. :)

If you like, I can include you on code reviews or cc for these things. I'm submitting on Gerrit myself, so you have to register there for to be able to CC you.

Also, if you would like to work on this area to help speed it along I would be very glad to give you pointers!

Thanks

Marcel

mpvl pushed a commit that referenced this pull request Dec 6, 2019
Found this case when debugging the failed test for #200

It's failing because of 52b04d4#diff-104bcbf8410c0cd325212e1b0af431aeL90

Closes #207
#207

GitOrigin-RevId: a2c97b8
Change-Id: Ifd1c9994b3c09b01cea38ab500ebe77e9e7ede4d
Reviewed-on: https://cue-review.googlesource.com/c/cue/+/4341
Reviewed-by: Marcel van Lohuizen <mpvl@golang.org>
@mpvl mpvl changed the title Control task dependency using before and after fields cmd/cue/cmd: control task dependency using before and after fields Dec 6, 2019
@mpvl mpvl closed this in e3a7a72 Dec 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants