Create dog and dog/run packages #113

xsb · 2017-01-19T19:04:10Z

What

I have been working on this PR for the past weeks. Most code has been either moved or refactored. The main purpose of this change is to make future development easier, we reached a point in which some features were simply impossible to implement without a change like this being made first.

How

Dog (the cli tool) has been moved to cmd/dog so package main is there now.

The root directory is now the dog package. Dogtools (like the dog cli) can import it to be able to parse Dogfiles and run tasks. This package replaces the old ones (parser, types and execute). Now there are three types: Dogfile, Task and TaskChain, each of them with methods to support the same functionality as the previous version. Most of the previous functions had been moved into type methods. Some of them had been simplified.

A new package has been created to manage the low level functionality of task execution, it's called dog/run and can be found in the run/ directory. This package exposes the Runner interface with methods to start tasks, collect their outputs and wait for them to finish. At this moment it explicitly supports the same runners that were tested as of v0.3.0 (sh, bash, python, ruby, perl) but it opens the possibility of creating more exotic runners like docker, kubernetes, swarm, etc. New runners can be developed outside of dog/run and as long as they implement the Runner interface dog will support them.

Proper documentation has been added and can be read using Godoc. Let me know if something is not clear, having good docs is one of the main objectives of this code change.

Dogfile Spec changes

The specification has changed a little bit:

exec directive is now runner
run directive is now code

The fact that executors are now called runners implies changes all over the place. This is also the reason why the execute package is now the new dog/run package.

Naming the old run directive just code have two objectives. First we avoid ambiguity with the concept of runners. Second, future runners will have this directive as optional, something that could feel unnatural if the name was run. It never made sense to use a verb as a directive name.

I am also adding support for backwards compatibility when the spec changes so old Dogfiles will work as expected with this new dog version. Deprecation detection has been added at package level, and now dog uses them to print warnings on STDERR after parsing. Current Spec changes will break on version v0.6.0, two versions from now.

Implementation changes

Dog uses exit status 1 if the main program fails and exit status 2 if any of the task fails. This behaviour is copied from GNU Make. The actual task exit status is printed together with elapsed time if the ProvideExtraInfo flag (-i) is enabled.

Removed the concept of Dogfile level hierarchy, we keep task chain though. This means that we don't store the whole hierarchy anymore. What dog does now is: process it as a check/validation, and generate it when a task needs to run.

Added validation methods to both Dogfile and Task types.

The operating system is detected at package init time, and the default runner is chosen based on this. This will be more relevant in the future, as Windows (cmd runner) is not suported yet.

Support for dotenv has been removed, as it was confusing. Should we search for the .env file in the current dir where dog is launched? or in the Dogfile path? Maybe in the task workdir? I think we can live without this, but if someone wants it we should first decide the expected behaviour.

ProvideExtraInfo (controled by the -i/--info flag) is now a package level variable.

Renamed status code to exit status everywhere.

Other

I removed the Gitter chat link from the README because no one was using it. I will remove the account after this PR is merged. We can open it again in the future if needed.

The README file is (I hope) simpler than before.

CI (Travis) now also runs errcheck and staticcheck.

Dist task now uses a directory called dog-{version} instead of dist when compressing the release, so it will have the appropriate name when the final user uncompresses it. This was really annoying before.

New dog logo added to img/ to be later used in the README.

Added examples/ directory with the first example showing the use of Dog as a library.

Version bump to v0.4.0. Why changing the version number if there are no new features? The point is that I consider the fact that this can be used as a library a feature, something that was promised before but never truly worked.

Notes

The code that implements the cli is almost the same as before. I know it should be simplified but it can be done later.

I know there are no unit tests, and this is something I want to change from now on. My plan is to test the packages using go test and then start a test suite using aruba. During this refactor I have been using dog run-test-dogfiles and it gave me enough confidence to believe this PR have feature parity with v0.3.0.

Thanks for reading all this! I am sure this is full of novice errors, and I want to learn. What would you improve?

marc-gr · 2017-01-19T21:19:33Z

Wow this looks amazing 🎉, I will need a little bit of time to go through all of it though ⏳

divan · 2017-01-20T11:43:35Z

run/run.go

+
+// GetOutputScanners is a helper method that returns two bufio.Scanner objects,
+// one for stdout and one for stderr.
+func GetOutputScanners(r Runner) (stdoutScanner bufio.Scanner, stderrScanner bufio.Scanner, err error) {


I think, you don't need to use bufio.Scanner here, as the only usage (so far) for the scanned output is simply print it to the stdout (in chain.go). Scanner is a higher level abstraction for tokens of data (and I suppose, runners do not have any assumtions about the data that will be printed), it requires more memory and, well, it's just a new abstraction level between the task producing output and the actual printing to stdout.

Moreover, Scanner will fail if the text is bigger that 64K (it's documented, and it's the reasonable choice for most of the scanner uses). So if any task will dump 64K+1 bytes of text, dog will fail.

My suggestion is to use io.MultiReader to combine both, stdoutReader and stderrReader (as bufio.Reader, perhaps) into one, and then just use io.Copy to print it to the stdout in one goroutine on chain.go. MultiReader will return EOF as soon as all readers return EOF, which is exactly what we want.

Thanks @divan, I pushed your commit

cooldarkdryplace · 2017-01-21T21:11:47Z

chain.go

+
+		t, found := d.Tasks[name]
+		if !found {
+			return errors.New("Task " + name + " does not exist")


fmt is imported already, so maybe fmt.Errorf("Task %q does not exist", name) should be used.

cooldarkdryplace · 2017-01-21T21:13:05Z

chain.go

+			runner, err = run.NewPerlRunner(t.Code, t.Workdir, t.Env)
+		default:
+			if t.Runner == "" {
+				return fmt.Errorf("Runner not specified")


Nothing to format here. errors.New() should be enough.

cooldarkdryplace · 2017-01-21T21:19:01Z

examples/http-api/http-api.go

+
+	// Launch the HTTP server
+	http.HandleFunc("/", handler)
+	http.ListenAndServe(":8080", nil)


http.ListenAndServe() may return an error, should this be logged?

cooldarkdryplace · 2017-01-21T21:22:02Z

parse.go

+	Workdir string      `json:"workdir,omitempty"`
+}
+
+// Parse accepts a slice of bytes and parses it folloing the Dogfile Spec.


Typo in following.

cooldarkdryplace · 2017-01-21T21:54:07Z

chain.go

+	timeMsg := ""
+
+	if d.Hours() > 1.0 {
+		timeMsg += fmt.Sprintf("%1.0fh", d.Hours())


Maybe simple assignment here. No need to concatenate with empty string.

Fixed. Thanks @bilinguliar!

This reverts commit ead3340.

cooldarkdryplace · 2017-01-22T12:14:29Z

chain.go

 	}

 	if d.Minutes() > 1.0 {
-		timeMsg += fmt.Sprintf("%1.0fm", d.Minutes())


Concatenation was unnecessary only for hours. Because initially we had blank string. Now we always rewrite string variable which may not be the intention. Sorry for confusing you with unclear comment.

True! This code was written half a year ago and I forgot how it worked. I added a better comment explaining the format.

Since v0.4.0 Dog uses the new directive code to define code blocks, this replaces the old run directive. More detailed info can be found in the release notes of the v0.4.0 version https://github.com/dogtools/dog/releases/tag/v0.4.0 and in this pull request dogtools/dog#113 At this moment Dog supports backwards compatibility, changes will break on v0.6.0.

xsb requested review from a user and marc-gr January 19, 2017 19:04

Create dog and dog/run packages

e2c4f09

xsb force-pushed the the-big-refactor branch from 697a309 to e2c4f09 Compare January 19, 2017 20:45

xsb added dogfile spec enhancement refactor labels Jan 19, 2017

Add example exposing tasks through HTTP

b338de0

divan reviewed Jan 20, 2017

View reviewed changes

CombinedOutput returns an io.Reader

bd0ec8d

cooldarkdryplace reviewed Jan 21, 2017

View reviewed changes

xsb added 5 commits January 22, 2017 12:54

Format error using fmt.Errorf

e4a3f59

Add error handling to ListenAndServe

36b1976

Typo

efcfb59

Remove unnecessary string concatenation

ead3340

Revert "Remove unnecessary string concatenation"

553a6ae

This reverts commit ead3340.

cooldarkdryplace reviewed Jan 22, 2017

View reviewed changes

xsb added 5 commits January 22, 2017 14:06

Fix unnecessary concatenation, improve code comment.

e06469c

Add fail if task does not exist at package level

e1d5362

No need to declare timeMsg variable

4446f4c

Run accepts two io.Writers to collect stdout and stderr separately

5cb24e3

Update examples, now passing stdout and stderr to Run()

812f202

xsb merged commit 8b696fb into master Jan 30, 2017

xsb removed request for a user and marc-gr January 30, 2017 21:15

xsb deleted the the-big-refactor branch January 30, 2017 23:12

xsb mentioned this pull request Feb 12, 2017

Update Dogfile to use 'code' instead of 'run' victorb/quickwiki#6

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create dog and dog/run packages #113

Create dog and dog/run packages #113

xsb commented Jan 19, 2017 •

edited

Loading

marc-gr commented Jan 19, 2017

divan Jan 20, 2017 •

edited

Loading

xsb Jan 20, 2017

cooldarkdryplace Jan 21, 2017

cooldarkdryplace Jan 21, 2017

cooldarkdryplace Jan 21, 2017

cooldarkdryplace Jan 21, 2017

cooldarkdryplace Jan 21, 2017

xsb Jan 22, 2017

cooldarkdryplace Jan 22, 2017

xsb Jan 22, 2017

Create dog and dog/run packages #113

Create dog and dog/run packages #113

Conversation

xsb commented Jan 19, 2017 • edited Loading

What

How

Dogfile Spec changes

Implementation changes

Other

Notes

marc-gr commented Jan 19, 2017

divan Jan 20, 2017 • 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

xsb commented Jan 19, 2017 •

edited

Loading

divan Jan 20, 2017 •

edited

Loading