Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Always print warnings and errors; clean up format #2213

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Always print warnings and errors; clean up format #2213

wants to merge 16 commits into from
+195 −33

Conversation

denik
Copy link
Contributor

@denik denik commented Jan 23, 2025 •
edited
Loading

Changes

  • Print warnings and errors by default.
  • Fix ErrAlreadyPrinted not to be logged at Error level.
  • Format log messages as "Warn: message" instead of "WARN" to make it more readable and in-line with the rest of the output.
  • Only print attributes (pid, mutator, etc) and time when the overall level is debug (so --debug output has not changed much).

Tests

  • Existing acceptance tests show how warning messages appear in various test case.
  • Added new test for --debug output.
  • Add sort_lines.py helper to avoid dependency on 'sort' which is locale-sensitive.

@denik denik temporarily deployed to test-trigger-is January 23, 2025 10:28 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is January 23, 2025 10:32 — with GitHub Actions Inactive
@denik denik force-pushed the DECO-24321---ErrAlreadyPrinted branch from 0f46e56 to efc48f1 Compare January 23, 2025 11:00
@denik denik temporarily deployed to test-trigger-is January 23, 2025 11:00 — with GitHub Actions Inactive
@denik denik force-pushed the DECO-24321---ErrAlreadyPrinted branch from efc48f1 to 131b414 Compare January 24, 2025 16:31
@denik denik temporarily deployed to test-trigger-is January 24, 2025 16:32 — with GitHub Actions Inactive
@denik denik changed the base branch from main to denik/selftest January 24, 2025 16:32
@denik denik temporarily deployed to test-trigger-is January 24, 2025 16:36 — with GitHub Actions Inactive
@denik denik force-pushed the DECO-24321---ErrAlreadyPrinted branch from 35ef576 to ff95005 Compare January 24, 2025 17:40
@denik denik temporarily deployed to test-trigger-is January 24, 2025 17:40 — with GitHub Actions Inactive
@denik denik changed the base branch from denik/selftest to denik/acc-diagnostics January 24, 2025 17:40
@denik denik changed the title WIP - logging Print warnings and higher by default; simplify format Jan 24, 2025
@denik denik temporarily deployed to test-trigger-is January 24, 2025 18:45 — with GitHub Actions Inactive
@denik denik changed the title Print warnings and higher by default; simplify format Always print warnings and errors; simplify format Jan 24, 2025
@denik denik force-pushed the DECO-24321---ErrAlreadyPrinted branch from 4406d92 to 7d12ede Compare January 24, 2025 19:20
@denik denik temporarily deployed to test-trigger-is January 24, 2025 19:20 — with GitHub Actions Inactive
@denik denik changed the base branch from denik/acc-diagnostics to denik/selftest January 24, 2025 19:21
Base automatically changed from denik/selftest to main January 27, 2025 08:17
@denik denik force-pushed the DECO-24321---ErrAlreadyPrinted branch from 7d12ede to b57aaf3 Compare January 27, 2025 08:33
@denik denik temporarily deployed to test-trigger-is January 27, 2025 08:33 — with GitHub Actions Inactive
@denik denik mentioned this pull request Jan 27, 2025
Merged
@denik denik force-pushed the DECO-24321---ErrAlreadyPrinted branch from b57aaf3 to cf5d64b Compare January 27, 2025 08:44
@denik denik temporarily deployed to test-trigger-is January 27, 2025 08:44 — with GitHub Actions Inactive
@denik denik force-pushed the DECO-24321---ErrAlreadyPrinted branch from cf5d64b to 6e50210 Compare January 27, 2025 08:56
@denik denik temporarily deployed to test-trigger-is January 27, 2025 08:56 — with GitHub Actions Inactive
@denik denik changed the title Always print warnings and errors; simplify format Always print warnings and errors; clean up format Jan 27, 2025
@denik denik temporarily deployed to test-trigger-is January 27, 2025 09:01 — with GitHub Actions Inactive
@denik denik marked this pull request as ready for review January 27, 2025 09:01
@denik denik requested a review from pietern as a code owner January 27, 2025 09:01
lennartkats-db
lennartkats-db reviewed Jan 29, 2025
View reviewed changes
libs/flags/log_level_flag.go
@@ -25,7 +25,7 @@ type LogLevelFlag struct {

func NewLogLevelFlag() LogLevelFlag {
return LogLevelFlag{
l: log.LevelDisabled,
l: log.LevelWarn,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Overall I like this PR a lot, @denik!

It's kind of hard to make sure that we don't show lots of undesired output suddenly, and the acceptance tests help a lot with that.

andrewnester
andrewnester reviewed Jan 29, 2025
View reviewed changes
integration/bundle/deploy_test.go
@@ -226,6 +226,7 @@ func TestDeployBasicBundleLogs(t *testing.T) {

stdout, stderr := blackBoxRun(t, ctx, root, "bundle", "deploy")
assert.Equal(t, strings.Join([]string{
fmt.Sprintf("Error: failed to delete /Workspace/Users/%s/.bundle/%s/artifacts/.internal: unable to delete filer root", currentUser.UserName, uniqueId),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounds a bit dangerous to call Delete on root

It's not on root but on <root>/.internal which is a special folder we create so we can safely remove it. As to keeping stuff around vs. cleaning up let's have a discussion as both have pros and cons

acceptance/bundle/git-permerror/output.txt
@@ -16,6 +17,7 @@ Found 1 error
Exit code: 1

>>> $CLI bundle validate -o json
Warn: failed to read .git: unable to load repository specific gitconfig: open config: permission denied
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the warning printed to stdout or stderr here? If stdout then it will be a breaking changing for people relying on JSON output here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stderr

@denik denik force-pushed the DECO-24321---ErrAlreadyPrinted branch from b8390b8 to 5fa68d7 Compare January 29, 2025 22:13
@denik denik temporarily deployed to test-trigger-is January 29, 2025 22:13 — with GitHub Actions Inactive
@denik denik force-pushed the DECO-24321---ErrAlreadyPrinted branch from 5fa68d7 to 472a5c3 Compare January 30, 2025 14:02
@denik denik temporarily deployed to test-trigger-is January 30, 2025 14:02 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is January 31, 2025 09:21 — with GitHub Actions Inactive
denik added 15 commits January 31, 2025 22:42
@denik
Show warnings+ by default
22ca8b1
@denik
Do not show error for ErrAlreadyPrinted (it's already printed)
e53d076
@denik
Downgrade Error: message after evert mutator to Debug
70cf048 
This seems redundant - nothing is lost.
@denik
tweak log output
e7b4846 
- remove time
- remove attrs unless level is debug
- replace $DEV_VERSION
- add acceptance test for --debug
@denik
update badness
5d41896
@denik
Restore back time for debug mode
647fe9f
@denik
fix slash on Windows
f09400e
@denik
do not use 'sort' - produces different result on Windows
5c046e7 
Might be fixable with Locale setting (https://stackoverflow.com/a/39172728), but it's easier just not to depend on it.
@denik
ruff format
51c8205
@denik
downgrade "failed execution" to Info
b0e3e01
@denik
update TestDeployBasicBundleLogs
44cce42
@denik
rebase and update acceptance tests
8fc1cf4
@denik
fix bundle/debug
724c68b
@denik
rebase & update acceptance tests
7530cae
@denik
update badness
f7e95e3
@denik denik force-pushed the DECO-24321---ErrAlreadyPrinted branch from 5149c9a to f7e95e3 Compare January 31, 2025 21:42
@denik denik temporarily deployed to test-trigger-is January 31, 2025 21:42 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is February 3, 2025 08:31 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewnester andrewnester andrewnester approved these changes

@shreyas-goenka shreyas-goenka Awaiting requested review from shreyas-goenka shreyas-goenka is a code owner

@pietern pietern Awaiting requested review from pietern pietern is a code owner

@lennartkats-db lennartkats-db Awaiting requested review from lennartkats-db

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@denik @pietern @andrewnester @lennartkats-db