Control astro dev Output with --verbosity Flag#1770
Conversation
schnie
force-pushed
the
feature/minimize-default-dev-output
branch
4 times, most recently
from
dd00790 to
bd1590d
Compare
schnie
force-pushed
the
feature/minimize-default-dev-output
branch
from
bd1590d to
4896922
Compare
schnie
force-pushed
the
feature/minimize-default-dev-output
branch
from
b295a68 to
6fa7960
Compare
| // Get project containers | ||
| psInfo, err := d.composeService.Ps(context.Background(), d.projectName, api.PsOptions{ | ||
| All: true, | ||
| }) | ||
| if err != nil { | ||
| return errors.Wrap(err, composeCreateErrMsg) | ||
| } | ||
| if len(psInfo) > 0 { | ||
| // Ensure project is not already running | ||
| for i := range psInfo { | ||
| if checkServiceState(psInfo[i].State, dockerStateUp) { | ||
| return errors.New("cannot start, project already running") | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
It doesn't seem necessary to throw an error if the project is already started. Let's just make sure the image is updated and started, then re-print the status info.
schnie
changed the title
astro dev Output with --verbosity Flag
schnie
changed the title
astro dev Output with --verbosity Flagastro dev Output with --verbosity Flag
schnie
force-pushed
the
feature/minimize-default-dev-output
branch
from
ca69f47 to
9e815ff
Compare
schnie
force-pushed
the
feature/minimize-default-dev-output
branch
from
9e815ff to
0250f96
Compare
schnie
requested review from
jeremybeard,
kushalmalani,
lzdanski and
neel-astro
as code owners
jeremybeard
left a comment
jeremybeard
left a comment
There was a problem hiding this comment.
Very nice UX improvement! Approving ahead of some small comments.
airflow/docker.go
Outdated
| return err | ||
| } | ||
|
|
||
| spinner.StopWithCheckmark(s, "Project has been started") |
There was a problem hiding this comment.
Happy to defer to others with more technical prose expertise but "has been" feels a bit redundant to me, would have expected just "Project started", and similarly for others below. I'd call this a nitpick but it becomes a prominent part of the CLI output now we're hiding so much low-level cruft.
airflow/docker_image.go
Outdated
|
|
||
| func (d *DockerImage) Pytest(pytestFile, airflowHome, envFile, testHomeDirectory string, pytestArgs []string, htmlReport bool, buildConfig airflowTypes.ImageBuildConfig) (string, error) { | ||
| // Determine if we should output to terminal or buffer. | ||
| output := logger.GetLevel() >= logrus.DebugLevel |
There was a problem hiding this comment.
We could add logger.IsLevelEnabled to pkg/logger and just call that from the if statements?
| configSetDefaultWorkspace = "\"%s\" Workspace found. This is your default Workspace.\n" | ||
|
|
||
| registryAuthSuccessMsg = "\nSuccessfully authenticated to Astronomer" | ||
| registryAuthSuccessMsg = "Successfully authenticated to Astronomer" |
There was a problem hiding this comment.
Thanks for cleaning up these confusing "\n... lines!
pkg/logger/logger.go
Outdated
| func IsLevelEnabled() bool { | ||
| return GetLevel() >= logrus.DebugLevel | ||
| } |
There was a problem hiding this comment.
@pritt20 what do you think about making the level a parameter here, so we could use it elsewhere to check the level? Maybe we'd want to rename to IsLevelAbove() or something. It just wasn't clear to me what this function name would be doing without a parameter passed to it. Or we could just name this function like IsAboveDebugLevel() and leave as-is. Curious what you think.
There was a problem hiding this comment.
Thanks @schnie !
I think it makes sense to convert this function to be more generic by accepting level as parameter. This would make this function flexible and more reusable across board.
I've made the required changes now.
There was a problem hiding this comment.
Can we just pass it on to the logrus IsLevelEnabled?
There was a problem hiding this comment.
Thanks for pointing out @jeremybeard ! I've made the above changes.
Co-authored-by: Pritesh Arora <[email protected]>
3 participants
Description
This PR cleans up the output on the
astro dev start/stop/kill/restartcommands. Today these commands output a lot of information that is mostly meaningless or unhelpful, distracting from the experience. A new user whose just downloaded the CLI should be focused on getting something running in Airflow, not thinking about containers and docker, etc yet.In this PR, we've suppressed the Image Build and Compose output for standard usage without any additional flags. This provides a nice, clean output with information about what's happening, but not every single detail. We're re-using the spinner we introduced recently for the runtime initialization messaging.
If users are interested in the full output, we've moved it behind the
--verbosity (debug|trace)flag. For example, if you are troubleshooting something deep in the stack for some reasonastro dev start --verbosity debugwill show the output, just as it did before this change.If the build phase encounters an error, which is probably a somewhat common occurrence in the wild, the
stdoutandstderrstreams will be printed out, along with the error message.🎟 Issue(s)
Related to https://github.com/astronomer/astro/issues/26133
🧪 Functional Testing
📸 Screenshots
Default experience
--verbosity debug|traceexperienceError on Build Default Experience
📋 Checklist
make testbefore taking out of draftmake lintbefore taking out of draft