refactor: rename logEntry to log and require for tasks, cmds and actions

[edvald]

This leverages the recent changes to the logger that allow for empty
log entries.

Note: This currently has an issue where the section part of log entries doesn't appear. Need input from @eysi09 to complete this.

[eysi09]

I haven't been able to reproduce the problem with the sections. Something like emptyEntry.setState({ section: mySection, msg: "hello" }) works on my end. Were there any particular steps to reproduce?

The issue with indentation of empty entries has been fixed in #302.

[eysi09]

I should also point out that appending nodes to an empty node will indent the child node with respect to the empty parent.

So this:

const empty = garden.log.info()
empty.setState("I'm not indented")
empty.info("I am (since this creates a new, non-empty, child node)")

prints this:

I'm not indented
    I am (since this creates a new, non-empty, child node)

...once #302 has been merged.

[edvald]

The issue isn't with setState, it's when I create a new line:

const log = garden.log.info()
log.info({ section: "foo" })

I'd expect this to print the full log line, and not omit the section bit.

[eysi09]

You're right. Sorry. Fixed in #316.

[eysi09]

Creating this entry here causes some log output to be printed before the command header. Like here:

g build
    ✖ openfaas--builder         → Building
    ✔ hello-npm-package         → Building → Done (took 0.1 sec)
    ✔ hello-container           → Building hello-container:vd8c433df4d... → Done (took 1.2 sec)
Build 🔨```

[edvald]

Good catch. I'll clean that up, and look for similar issues.

[eysi09]

There are currently two issues:

Issue 1

The commands use a placeholder log entry (and pass them onwards) whereas the Garden class uses the root logger (and passes it onwards). This means that everything logged by the Garden class, and all the methods that receive the root logger from it, will always be either above or below everything that descends from the command placeholder entry.

This results in something like this:

Logging stuff from the Garden class
    Build 🔨

    ✔ go-service                → Building go-service:611c61cddc... → Done (took 0.5 sec)

    Done! ✔️

or something like this:

    Build 🔨

    ✔ node-service              → Building node-service:611c61cddc... → Done (took 0.7 sec)
    ✔ go-service                → Building go-service:611c61cddc... → Done (took 0.8 sec)

    Done! ✔️

Logging stuff from the Garden class

depending on whether or not the placeholder log entry is created before or after the Garden class is initialised. In the second example the "Logging stuff..." text gets repeatedly pushed down as more content is printed above which looks a little bit awkward.

Solution

We can of course move the Build and Done messages around to ensure they always get printed first and last. By I just wanted to double check that this is indeed the intended behaviour.

Issue 2

The logs printed by the placeholder log entry all get indented since they're the children of the empty entry. This is because the original motivation behind allowing entries to create child entries was to create subtasks.

Solutions

a) Never indent subtasks unless the option is explicitly set (we'd need to update all current subtasks)
b) Check if parent is empty and if so skip indentation at the render level. This feels a bit hacky since someEntry.info("am I indented?") might either be indented or not, depending on how someEntry was created.
c) Continue to distinguish between the logger and individual log entries. This would be the most idiomatic way imo.
d) Set indentation to -1 on the empty entry that gets passed to the commands. This is probably the most simple solution.

Note that setState prevents indenting since that just updates the current (possibly empty) entry. However, that won't work if we need to create child entries from that entry.

[eysi09]

I just update the branch with a fix for issue 2, opting for solution d).

So now the main concern is how we handle issue 1.

[edvald]

Re Issue 1, I think we should have Garden accept a LogEntry instead of a Logger. I think that would sort the issue out more neatly. Then garden.log does the expected thing.

[eysi09]

Updated the PR with a commit that I think should solve most of our issues for now.

I actually went ahead and remove the ability to create an empty log entry like so: logger.info() or logEntry.info() and instead added a placeholder method that does the same with the following signature: placeholder(level: LogLevel = LogLevel.info): LogEntry.

I think that makes things a little bit clearer. My main concern was that a function that receives a log entry doesn't really know what to do with it, since it wasn't clear from the context whether the entry was simply an empty placeholder that should be populated or a task with a spinner that should be updated. Type safety-wise, this is still an issue but at least the intent is a little bit clearer.

I think it would make sense to continue to use the logger as before within the commands and have them set up the entries that get passed to the tasks and actions. That could be an intermediary step before we implement something event based, but I skipped that for now so that we can finally get this merged.

Regarding the log entry control flow in general, this is kind of how I envisioned it (and why I've been on the fence about making everything a log entry):

const entry = logger.info({msg: "running task..." status: "active"})
try {
  // pass the entry along so that it can be updated while task is running
  runTask(entry)
} catch (err) {
  entry.setState(err.message)
}
entry.setSuccess("whoop")

However, the entries are kind of all over the place right now. Also, when everything is an entry, you might end up calling setState on empty placeholders which doesn't really make sense.

But as I mentioned above, I think some of this can be reigned in by initialising the placeholder entries at the command level and structure the whole thing a bit more.

[eysi09]

@edvald, if the last commit looks okay to you I can go ahead and squash and merge.

[eysi09]

...or perhaps I'll leave is as two commits since there's a lot of conversation around this.

[edvald]

👍 approved