-
-
Notifications
You must be signed in to change notification settings - Fork 580
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
API/json implementation and interaction with ddev, fixes #477 #499
Conversation
ef29020
to
c73bbd4
Compare
This still needs tests for the json output, and I'll be doing a walkthrough of the interactive prompts like ddev config, but the basics are in place and it passes tests. I'd really appreciate a review of the code and approach and of the json output. I'm updating the OP to describe what's been done and what it means, and the structure of the json ouput. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a first pass through the code here... there's a bit to review, so this isn't a thorough pass by any means.
I'm kind of surprised at how much utility function this has required. When I did some fancier logging with logrus in asset-manager, I was able to keep using log.Print et al as usual, and direct the output the way I needed to at the end. However, I didn't have to alter my output or deal w/ json (though i expect I will before long there), so I expect there's plenty more complications you've faced on this than I did on that other project. Not sure what I'm after here, perhaps a bit of explanation could help, but mostly just reacting to how much this effort ended up taking.
I haven't run this yet, but I'll take a stab at it tomorrow or the next with fresher eyes. Overall my initial gut reaction is that this approach seems fine, just surprising how much of the details we're having to address on our own.
cmd/ddev/cmd/root.go
Outdated
@@ -125,3 +117,7 @@ func dockerNetworkPreRun() { | |||
util.Failed("Unable to create/ensure docker network %s, error: %v", netName, err) | |||
} | |||
} | |||
|
|||
func init() { | |||
RootCmd.PersistentFlags().BoolVarP(&util.JSONOutput, "json", "j", false, "If true, iser-oriented output will be in JSON format.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about output-json or json-output? Not sure we need a shorthand flag since users are unlikely to use this flag, though it doesn't hurt either.
Typo: s/iser-oriented/user-oriented
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open to --json-output. -j is in fact nice for testing.
pkg/ddevapp/config.go
Outdated
@@ -230,10 +230,12 @@ func (c *Config) Read() error { | |||
// WarnIfConfigReplace just messages user about whether config is being replaced or created | |||
func (c *Config) WarnIfConfigReplace() { | |||
if c.ConfigExists() { | |||
util.Warning("You are re-configuring the app at %s. \nThe existing configuration will be updated and replaced.\n\n", c.AppRoot) | |||
util.Warning("You are re-configuring %s. The existing configuration will be replaced.\n\n", c.AppRoot) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only need one of these :) First or last ones look best to me.
pkg/plugins/platform/local.go
Outdated
exec = append(exec, cmd...) | ||
|
||
return dockerutil.ComposeCmd(l.ComposeFiles(), exec...) | ||
} | ||
|
||
// ExecWithTty executes a given command in the container of given type. It allocates a pty for interactive work. | ||
// Returns ComposeCmd results of err, stdout, stderr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment line seems inaccurate for this function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 7eb90e2
pkg/plugins/platform/local.go
Outdated
@@ -667,7 +661,7 @@ func (l *LocalApp) DockerEnv() { | |||
// @ TODO: I have no idea what a Setenv error would even look like, so I'm not sure what |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, we can probably lose this TODO. I think we're doing all that's reasonable to be done here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey we've had that in there so long it's an old friend!
@@ -219,13 +223,11 @@ func TestLocalImportDB(t *testing.T) { | |||
err = app.ImportDB(cachedArchive, "") | |||
assert.NoError(err) | |||
|
|||
stdout := testcommon.CaptureStdOut() | |||
err = app.Exec("db", true, "mysql", "-e", "SHOW TABLES;") | |||
out, _, err := app.Exec("db", "mysql", "-e", "SHOW TABLES;") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the 2nd returned var actually used anywhere we're calling app.Exec? (stderr output i think) I don't think I've seen it in use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the tests now use it instead of trying to capture output in the fancy output-capture way.
pkg/util/log_support.go
Outdated
return true | ||
} | ||
for _, ch := range text { | ||
if !((ch >= 'a' && ch <= 'z') || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm having a hard time grokking these conditions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah copied from logrus. No idea.
pkg/util/log_support.go
Outdated
if !f.DisableTimestamp { | ||
f.appendKeyValue(b, "time", entry.Time.Format(timestampFormat)) | ||
} | ||
//f.appendKeyValue(b, "level", entry.Level.String()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, cleanup - this and a couple lines later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in fa6049a
pkg/util/log_support.go
Outdated
} | ||
} | ||
|
||
// Below is from logus json_formatter.go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be useful to reference a commit/file this is adapted from, and to highlight any of our customizations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in 634f884
func Failed(format string, a ...interface{}) { | ||
Error(format, a...) | ||
os.Exit(1) | ||
if a != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure i understand the reason for condition check here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRc, log.Fatalf("some format string", nil) makes ugly output, pointing out the nil to the end user and calling it bad.
pkg/util/utils.go
Outdated
} | ||
|
||
// Error will print an red error message but will not exit. | ||
func Error(format string, a ...interface{}) { | ||
color.Red(format, a...) | ||
if a != nil { | ||
UserOut.Fatalf(format, a...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fatal doesn't seem right here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Fixed in 210a22b
7a53cfa
to
40b59f0
Compare
This now has tests for |
29bb61e
to
04c3d9b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a first pass on the code. Things look like they are moving the right direction but I did have some feedback. Thanks for the work so far on this.
pkg/util/log_support.go
Outdated
@@ -0,0 +1,347 @@ | |||
package util |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would probably feel better to me as it's own package. It feels sort of weird to see util.UserOut
everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestions for package name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logger
, output
, format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There goes Brad wanting a package for every function :)
func describeApp(appName string) (string, error) { | ||
var err error | ||
// renderAppDescribe takes the map describing the app and renders it for plain-text output | ||
func renderAppDescribe(desc map[string]interface{}) (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this functionality probably deserves a life outside of our cobra commands.
I'd probably prefer output format as an argument to site.Describe()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can move this. However, it's only a text formatting element, the same as what was here before, and it would only be used by this cobra command. It takes the authoritative description (a map) and turns it into a text table (which would only be used by the cobra command).
As far as the usage of site.Describe() I guess I disagree. It now gives an authoritative machine-usable response (a map), which can be turned into a text table. Having it provide both a textual representation and a map would require extra processing for every call (no big deal) and have 2 returns (odd). But the only place it gets turned into a table is here.
cmd/ddev/cmd/list.go
Outdated
} | ||
|
||
util.UserOut.WithField("raw", appDescs).Print(table) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar to above, it feels like would be better served with a single output flag to CreateAppTable()
. If we ever consume those packages from another source. JSON output could very well be important to what we are doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The table is unrelated to the actual json that is output here. There's a kind of a trick going on with this output: The 'raw' field is for the json consumer, and the table is for the plain-text consumer.
|
||
// Look through list results in json for this site. | ||
found := false | ||
for _, listitem := range raw { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it would be easier/more clear to just define the struct you are looking here and compare to the values in raw
using reflect.DeepEqual()
? https://golang.org/pkg/reflect/#DeepEqual
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good suggestion for a thorough test. My opinion is that this is just a one-off test and whatever works is OK here. This is "good enough" for now. It's a slight improvement on what we had.
pkg/ddevapp/providerPantheon.go
Outdated
"os" | ||
"path/filepath" | ||
"strings" | ||
|
||
log "github.com/sirupsen/logrus" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a log.Fatalf
in this file which I believe should use the new output logger?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in b1367ee
} | ||
appTable := CreateAppTable() | ||
shortRoot := RenderHomeRootedDir(l.AppRoot()) | ||
appDesc := make(map[string]interface{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would feel a lot better if were a real type rather than map[string]interface
. The same struct could be used (or we could use a trimmed down version and rely on composition) for the ddev list
values.
pkg/plugins/platform/local.go
Outdated
fmt.Println("Provide the path to the database you wish to import.") | ||
fmt.Println("Import path: ") | ||
util.UserOut.Println("Provide the path to the database you wish to import.") | ||
fmt.Print("Import path: ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be util.UserOut
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately the logrus system always seems to emit a CR, so I used fmt.Print() on the prompts for config and import commands. Open to other solutions. The reality is we need to use one of many prompter libraries. Opened #523 for this.
pkg/plugins/platform/local.go
Outdated
fmt.Println("You provided an archive. Do you want to extract from a specific path in your archive? You may leave this blank if you wish to use the full archive contents") | ||
fmt.Println("Archive extraction path:") | ||
util.UserOut.Println("You provided an archive. Do you want to extract from a specific path in your archive? You may leave this blank if you wish to use the full archive contents") | ||
fmt.Print("Archive extraction path:") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be util.UserOut
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, same reason, apparently no way to get Logrus to not emit a CR, so prompting looks weird.
pkg/util/log_support.go
Outdated
} | ||
|
||
// LogSetUp sets up UserOut and log loggers as needed by ddev | ||
func LogSetUp() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: This feels more like "output" than "logs" to me. I'm not sure why. Maybe there's no difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the perspective of Logrus, it's log setup. From the perspective of the end user it's output.
cmd/ddev/cmd/root.go
Outdated
@@ -125,3 +117,7 @@ func dockerNetworkPreRun() { | |||
util.Failed("Unable to create/ensure docker network %s, error: %v", netName, err) | |||
} | |||
} | |||
|
|||
func init() { | |||
RootCmd.PersistentFlags().BoolVarP(&util.JSONOutput, "json-output", "j", false, "If true, user-oriented output will be in JSON format.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to leave the door open to future output formats. I'd probably prefer one of the following methods here:
- Drush style:
--format=json
- kubectl/helm golang style:
-o/--output=json
I think all the small feedback is accounted for. Now we have decisions to make on larger feedback:
We should have a quick chat or meeting to resolve these, and I think this will be ready for a final pass when it's green (I seem to have broken something in a few trivial commits). |
d2a785b
to
d279609
Compare
Regarding the 4 open items in #499 (comment), for items 1 and 2 I'm all in favor of simplicity for now and expanding when necessary. So if we decide to just keep it a boolean as a toggle and use the existing structure, for now, that probably makes sense unless/until we need the additional effort. I have no opinion on items 3 and 4. |
There is one more test fix for windows required on this. It affects nothing for review or decisionmaking. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took this for a spin and mostly saw things working as expected. Thanks for all the work on this so far. I did find some areas where normal text output wasn't what I expected:
-
I have to confess, I miss the container output during startup. We don't have any new output until the site is fully ready, and it makes the wait rather uncomfortable.
-
ddev rm and ddev stop give no output until they return successful (or not). Empty newline after the success message on rm.
-
ddev start output was all white text. Previously success messages were colored blue.
-
unexpected newline after ddev exec output
-
ddev hostname error outputs as "FATA[0000] Could not add hostname"
-
Seems like success messages in general are not output as colored text
pkg/testcommon/testcommon.go
Outdated
// Capturing starts when it is called. It returns an anonymous function that | ||
// when called, will return a string containing the output during capture, and | ||
// revert once again to the original value of os.StdOut. | ||
func CaptureUserOutOutput() func() string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this name really stutters. CaptureUserOut would be fine IMO
pkg/util/log_support.go
Outdated
levelColor = nocolor | ||
} | ||
|
||
fmt.Fprintf(b, "\x1b[%dm%s\x1b[0m ", levelColor, entry.Message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be helpful to have explanations like this in code comments. I still don't have a firm grasp of what's copied as-is from logrus vs what's been modified, and it'd help to be able to make this determination quickly months later when we've forgotten this discussion.
@tannerjfco Item #4 was based on an earlier discussion in this issue, #499 (comment) - do you want to pursue that at all? We could just use log and lose the "native" capabilities/format of log.Debug() and such. It would make the code less verbose, which would be nice. But then log() is also a kind of not doing log() things at all. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spent about an hour with this today in both JSON and standard output modes. I was able to find a few issues, but nothing that wasn't covered in @tannerjfco's feedback.
I'm good with this once that feedback is addressed.
42863d3
to
80ed3bc
Compare
80ed3bc
to
4f35405
Compare
I think this is ready for maybe a final test. It seems like the important issues are taken care of.
For this, I made ... output appear to normal users, but the original docker-compose output show if we have DRUD_DEBUG set. That's an experiment (both ways) to see if you like it, but I like it a little better. I was also missing the time-based feedback.
Fixed
This is definitely different than previously, and I was unable to figure out how to make it be the same as before, but I'm not convinced it has to be. The bottom line issue is that we only have one type of output that can be mapped with "success" or "neutral" output, and that's logLevel.info. And we use log-type output for everything that's important. So there was previously an implicit difference (or one-off difference) between things that were explicitly "success" and those that were just informational, but it was never a specific practice. I'm hoping you'll agree to go with this as it is.
I apologize that I don't know how to do with with logrus-based output. Since it was designed for logging, it always outputs a newline, and the result of the command in the container also outputs a newline. There are potential workarounds, but I don't like them much.
Fixed. And thanks especially for this one. It pointed out a very strange setup on the hostname command. |
8e63531
to
0d16773
Compare
Unfortunately the progress dot output didn't seem to work very well for me. When I ran "ddev start" I got 2 dots, then nothing followed by a bit of wait, and then suddenly the success output emitted. When I ran "ddev rm" no progress dots emitted until the command returned successful. Overall, I gotta say I really prefer having the compose output. I think its useful to know what its doing, in this case starting containers. On a related note, I also find the "Starting health checks" useful, as we're waiting here for things to report back ok as well. Without this output, we indicate that containers are likely past starting, but no indication of what we're waiting for from there. I'm open to a change in message here, as I know you didn't consider this output useful, but I think its worth saying what we're waiting for.
I liked having success messages easily distinguished from the rest of the output, but you are right this is not a strict necessity. I'm willing to accept it and move on given the technical limitation involved, and I don't think it should be too disrupting for users. |
So @rmanelius I guess you'll have to make the call. @beeradb you should chime in. In the API stuff, to prevent uncontrolled output (and supposedly to improve user experience) I replaced all the incomprehensible docker-compose output (Starting this container, etc.) with just dots. DRUD_DEBUG=true brings the old stuff back. My intent was two-fold,
Now that I went back and built the capability of the dots, it is possible (and already implemented with DRUD_DEBUG=true) to output the stderr from docker-compose (for #1) But I still maintain that our users do not need to see that. So decision time. |
@rfay Acknowledged that this is a request for me to weigh in. Attempting to get a test on this today... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reference
Testing
Performed by Rick Manelius as of 2017-11-07
Hardware Setup
I’m using a laptop with the following high-level specifications:
- MacBook Pro: Retina 15-inch Mid 2015
- 2.2 Ghz Intel Core i7
- 16 GB 1600 MHz DDR3 RAM
- 250 GB Hard Drive
- macOS Sierra 10.12.4
Software Setup
- Xcode
- Go 1.8.1
- Docker 17.06.0-ce-rc1
- Git 2.11.0
Repositories Used
Setup
git fetch --all
git checkout 0d16773
make clean && make darwin
ddev version
outputs v0.9.3-14-g0d167733
Results
- Testing json output
- ddev list => works
- ddev describe => works
- ddev stop => works and provides more info as it's available
- Testing standard output
- For ddev start/stop/restart, I fall on the side of wanting to see the docker compose output. Three reasons:
- Basic debugging and alerting the end-user to containers being created/removed.
- Showing progress. This is particularly true for longer operations. Seeing absoultely nothing for up to say a minute can make it feel longer or makes me think something may be stuck.
- Verbosity. If we rely on DRUD_DEBUG, then I anticipate many will leave on by default and get overwhelmed.
- For ddev start/stop/restart, I fall on the side of wanting to see the docker compose output. Three reasons:
Recommendations
- I vote for bringing back that docker compose output that we used to have for start/stop/rm/restart.
- I agree that the success message coloration was used to have is preferrable, but I do believe there are related issues that we can address color consistency Assumption about setting colors in output is not valid; doesn't work in cmd on windows #363. To be explicit, I don't think this should hold up this PR from acceptance.
Unblocked me! Just so you know @rickmanelius I don't think the color issues here are related to #363. The problem here is that we don't have any way in the logging platform to distinguish between "informational" and "success". And since we're using consistent output techniques, we don't want to be randomly coloring things, but rather use the chosen output to do that. That said, it's probably possible to randomly and manually color a few outputs. |
I re-enabled output of docker-compose output. There is a newline (sometimes) output in the docker-compose output that I've been unable to track down and work on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the work on this @rfay. I think we're good to go here 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick turn! 👍
The Problem/Issue/Bug:
OP #477: It will be done in several sections:
Work to do from #477:
ddev list
andddev describe
need more attention than this because they painstakingly build the text as a simple output buffer. We'll need to refactor them so that they build a data structure and then either output the data structure as json or render the table and output it.Note that
ddev logs
turns output responsibility over to docker, so can't currently output in json. We need to consider whether we want to be able to turn container logs into json.Design and Structure
This PR changes ddev to use a single output mechanism for all user-destined output. The UserOut log object (based on spf13/logrus) has been modified to output directly to the user, except when the --json flag is provided, in which case it outputs a json response to the same command. So for example,
ddev list
gives traditional output (but using UserOut, not direct stdout), butddev list --json
orddev list -j
gives the same a machine-readable json version.JSON output
For the json
ddev list
andddev describe
commands a field named "raw" is added. This is what should be consumed by ddev-ui. The normal msg is there, but it's intended for non-json uses, and would be inappropriate to parse.For all other commands, the normal response (or error response) is just wrapped in a json envelope with severity, etc.
Manual Testing Instructions:
Try all commands. Try the commands with --json (or -j). The json output is pretty-printed when output to a terminal, to make it easier to analyze.
Automated Testing Overview:
Related Issue Link(s):
Release/Deployment notes: