Control `astro dev` Output with `--verbosity` Flag #1770

schnie · 2024-12-19T20:33:26Z

Description

This PR cleans up the output on the astro dev start/stop/kill/restart commands. 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 reason astro dev start --verbosity debug will 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 stdout and stderr streams will be printed out, along with the error message.

🎟 Issue(s)

Related to https://github.com/astronomer/astro/issues/26133

🧪 Functional Testing

List the functional testing steps to confirm this feature or fix.

📸 Screenshots

Default experience

`--verbosity debug|trace` experience

Error on Build Default Experience

📋 Checklist

Rebased from the main (or release if patching) branch (before testing)
Ran make test before taking out of draft
Ran make lint before taking out of draft
Added/updated applicable tests
Tested against Astro-API (if necessary).
Tested against Houston-API and Astronomer (if necessary).
Communicated to/tagged owners of respective clients potentially impacted by these changes.
Updated any related documentation

schnie · 2024-12-20T18:49:15Z

airflow/docker.go

-	// 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")
-			}
-		}
-	}
-


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.

jeremybeard

Very nice UX improvement! Approving ahead of some small comments.

jeremybeard · 2025-01-14T14:57:32Z

airflow/docker.go

@@ -279,6 +291,8 @@ func (d *DockerCompose) Start(imageName, settingsFile, composeFile, buildSecretS
 		return err
 	}

+	spinner.StopWithCheckmark(s, "Project has been started")


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.

jeremybeard · 2025-01-14T15:01:55Z

airflow/docker_image.go

 }

 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


We could add logger.IsLevelEnabled to pkg/logger and just call that from the if statements?

jeremybeard · 2025-01-14T15:02:23Z

cloud/auth/auth.go


-	registryAuthSuccessMsg = "\nSuccessfully authenticated to Astronomer"
+	registryAuthSuccessMsg = "Successfully authenticated to Astronomer"


Thanks for cleaning up these confusing "\n... lines!

pritt20

Thanks for these changes @schnie !! 🚀

schnie · 2025-01-16T12:58:46Z

pkg/logger/logger.go

+func IsLevelEnabled() bool {
+	return GetLevel() >= logrus.DebugLevel
+}


@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.

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.

Can we just pass it on to the logrus IsLevelEnabled?

Thanks for pointing out @jeremybeard ! I've made the above changes.

Co-authored-by: Pritesh Arora <[email protected]>

schnie force-pushed the feature/minimize-default-dev-output branch 4 times, most recently from dd00790 to bd1590d Compare December 20, 2024 04:25

Hides most build and start output behind verbosity level

4896922

schnie force-pushed the feature/minimize-default-dev-output branch from bd1590d to 4896922 Compare December 20, 2024 16:11

Merge branch 'main' into feature/minimize-default-dev-output

6fa7960

schnie force-pushed the feature/minimize-default-dev-output branch from b295a68 to 6fa7960 Compare December 20, 2024 18:47

schnie commented Dec 20, 2024

View reviewed changes

schnie changed the title ~~Hides most build and start output behind verbosity level~~ Minimize astro dev Output with --verbosity Flag Dec 20, 2024

schnie changed the title ~~Minimize astro dev Output with --verbosity Flag~~ Control astro dev Output with --verbosity Flag Dec 20, 2024

schnie force-pushed the feature/minimize-default-dev-output branch from ca69f47 to 9e815ff Compare December 20, 2024 19:18

Reorganize PersistentRunE hooks

0250f96

schnie force-pushed the feature/minimize-default-dev-output branch from 9e815ff to 0250f96 Compare December 20, 2024 19:43

schnie added 4 commits December 23, 2024 14:14

Switch env file logs to debug logger

d5c580c

Switches build to new spinner helpers

0a1fdc3

Cleanup auth output

f47a104

Merge branch 'main' into feature/minimize-default-dev-output

5c95c3f

schnie marked this pull request as ready for review January 12, 2025 17:48

schnie requested review from neel-astro, jeremybeard, kushalmalani and lzdanski as code owners January 12, 2025 17:48

Merge branch 'main' into feature/minimize-default-dev-output

993e136

schnie requested a review from pritt20 January 12, 2025 17:49

jeremybeard approved these changes Jan 14, 2025

View reviewed changes

Pritesh Arora added 2 commits January 15, 2025 18:36

Addressed feedback

056e8a1

Addressed feedback

4e07614

pritt20 approved these changes Jan 15, 2025

View reviewed changes

schnie commented Jan 16, 2025

View reviewed changes

Pritesh Arora added 2 commits January 16, 2025 19:12

Addressed feedback

87e99dc

clean up

5bb0acb

pritt20 merged commit a2a4c12 into main Jan 16, 2025
5 checks passed

pritt20 deleted the feature/minimize-default-dev-output branch January 16, 2025 17:03

neel-astro pushed a commit that referenced this pull request Jan 21, 2025

Control astro dev Output with --verbosity Flag (#1770)

6549eaf

Co-authored-by: Pritesh Arora <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Control `astro dev` Output with `--verbosity` Flag #1770

Control `astro dev` Output with `--verbosity` Flag #1770

schnie commented Dec 19, 2024 •

edited

Loading

schnie Dec 20, 2024

jeremybeard left a comment

jeremybeard Jan 14, 2025

jeremybeard Jan 14, 2025

jeremybeard Jan 14, 2025

pritt20 left a comment

schnie Jan 16, 2025

pritt20 Jan 16, 2025

jeremybeard Jan 16, 2025

pritt20 Jan 16, 2025


		registryAuthSuccessMsg = "\nSuccessfully authenticated to Astronomer"
		registryAuthSuccessMsg = "Successfully authenticated to Astronomer"

Control astro dev Output with --verbosity Flag #1770

Control astro dev Output with --verbosity Flag #1770

Conversation

schnie commented Dec 19, 2024 • edited Loading

Description

🎟 Issue(s)

🧪 Functional Testing

📸 Screenshots

Default experience

--verbosity debug|trace experience

Error on Build Default Experience

📋 Checklist

Choose a reason for hiding this comment

jeremybeard left a 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

pritt20 left a 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

Control `astro dev` Output with `--verbosity` Flag #1770

Control `astro dev` Output with `--verbosity` Flag #1770

schnie commented Dec 19, 2024 •

edited

Loading

`--verbosity debug|trace` experience