Rewrite Mayday

[tschuy]

I rewrote mayday so all file operations are done in /mayday.go. This means it's possible to test things in /mayday/* without touching the filesystem.

Architectural changes:

• move file handling to main mayday.go.
• pack things directly into tarball. Do not pass /tmp, do not collect $200.
• configuration file! This I'm not 100% sure on. Right now it's just being read from the filesystem, in ../config.json from wherever the binary mayday is built (defaulting to bin/mayday). Obviously this isn't ideal; where should I put this? Should it even be separate?

Other changes:

• remove Godeps/, add vendor/
• modify ./build: remove GOPATH export. Is this alright? It works on Travis ¯_(ツ)_/¯ What should I do with the other parts of the build script that mess with go env variables?
• bump Travis to Go 1.6, and remove 1.4/1.5. This is necessary with vendoring.
• report coverage as part of testing, because why not. 0% → 44% for github.com/coreos/mayday/mayday!

Things I'm not sure on:

• testing journal.go. We can't just assume a system has systemd on it, and even if we do, we still don't want to touch the filesystem. (I lied earlier, journal.go still touches the filesystem.)

[tschuy]

@brianredbeard

[colhom]

configuration file! This I'm not 100% sure on. Right now it's just being read from the filesystem, in ../config.json from wherever the binary mayday is built (defaulting to bin/mayday). Obviously this isn't ideal; where should I put this? Should it even be separate?

A few points here:

• default configuration file path should be absolute, not relative to binary. I'll throw out /etc/mayday.conf
• The readConfig() method here should take in an io.Reader interface type as an argument. The main() function should take care of opening the file, passing it to readConfig, and closing the file. We want config parsing to be unit testable.

What should I do with the other parts of the build script that mess with go env variables?

If you've switched from Godeps to using vendor and everything is working, good enough for now. Either @brianredbeard or I will clean up at a later date.

bump Travis to Go 1.6, and remove 1.4/1.5. This is necessary with vendoring.

I'm fine with go 1.6 or higher for our build environment.

testing journal.go. We can't just assume a system has systemd on it, and even if we do, we still don't want to touch the filesystem.

We'll treat journal.go as you would database access code or an API wrapper- we will consider it to be an external component and only seek to allow other components to unit test against it.

The way you have it is pretty ideal- it's basically just shelling out to journalctl... not really much to go wrong. We can perhaps write some integration tests which exercise it, but for now we'll just stub it out so other components can unit test against the Journal interface. (yes, more interfaces)

• Create an interface definition from the Journal struct function signature and call it Journal
• Rename the Journal struct to SystemdJournal. We could perhaps add more later to support other logging providers.
• In journal_test.go, define another struct DummyJournal which implements the Journal interface to your liking.

Great job intern.

[tschuy]

Ok, I've addressed pretty much all of the feedback except things relating to the journals (and the for loops). Tar now accepts a Tarable as its argument for .Add(), which is super nice.

[tschuy]

If I switch to for..range loops, then I have to deal with go copying the objects around like there's no tomorrow, which breaks some of my pointer passing because it reuses the objects it's copying into. it's easier to just use the "classic" for loops.

[tschuy]

Looking at your comments on Journal, now that it's just a Tarable like everything else the only parts there are to test are really

• Run(), which calls out to journalctl as pretty much the only thing it does. Not sure how testable this'd be without pretending every system has journalctl.
• Header(), which is totally testable, but a very simple function that's not really the priority in testing
• getJournals(), which essentially calls out to go-systemd/dbus, loops over them to get their names and, for each one in /usr/lib\*/systemd/system/\*.service, makes a SystemdJournal object. It doesn't touch the filesystem (except in its calls to dbus). I'll look into mocking the dbus calls, see if I can get this tested.

EDIT: Looked into mocking, it's stupendously easy in go, wow.

[tschuy]

Ok, this is ready to go pretty much I think. I didn't write any testing for the .Run() part of the journal, since as it turns out mocking is actually slightly harder than I thought with external libraries. (I'd have to do something like this.)

I'll keep poking at it, but for now I think I'm ready for this to be reviewed (and hopefully merged!)

[tschuy]

Ok, I've fixed the minor cleanup things (cbytes[:], for..range, renaming File to MaydayFile), and factored out SystemdJournal just a little bit so that it's slightly more testable.

We're up to:

• mayday/command.go: 82.9% (command timeout handling mostly, not sure how to test this?)
• mayday/tar.go: 81.8% (all error handling)
• mayday/journal.go: 38.6% (non-tested parts talk to dbus or to cmd.Execute)
• mayday/mayday.go: 0% (this is fairly trivial, I don't think it needs tests)
• mayday/file.go: 0.0% (this file is trivial, I don't think it really needs tests)
• mayday.go: 7.4% (the entire point of this file is handling filesystem operations)

[colhom]

LGTM!!

[colhom]

@tschuy maybe squash up into a few indicative milestones before the final merge. We have a lot of commits right now

[tschuy]

I've squashed everything into a slightly more reasonable number of commits. @colhom the only thing that's changed since you last checked it out is I got rid of .Run() as a Tarable interface method, and it's now called if necessary when accessing .Content() or .Header().

[brianredbeard]

LGTM