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.

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

Disable log coloring #1595

Merged

Conversation

bszirtes
Copy link
Contributor

Description

Introduced an option to disable the coloring of the log messages and thus remove the control characters responsible for it, via an optionally defined environmental variable.

Issue link

#1594

How Has This Been Tested?

  • Added unit testing to cover
  • Tested manually
  • Tested by integration testing
  • Have not tested

Types of changes

  • Bug fix
  • New functionality
  • Documentation
  • Refactoring
  • CI

Copy link

codecov bot commented Mar 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (main@b1a3e26). Click here to learn what that means.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1595   +/-   ##
=======================================
  Coverage        ?   67.31%           
=======================================
  Files           ?      261           
  Lines           ?    12380           
  Branches        ?        0           
=======================================
  Hits            ?     8333           
  Misses          ?     3524           
  Partials        ?      523           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines 37 to 39
if os.Getenv("LOG_NOCOLORS") == "true" {
f.nf.NoColors = true
}
Copy link
Member

Choose a reason for hiding this comment

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

Let's use this behaviour by default. I don't see any problems on our side. So if it's also fine for your side, let's do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you suggesting removing the verification process and adopting the f.nf.NoColors = true behavior as the default?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it's correct. I don't see any motivation for using these control symbols so we can just cut it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be fine from our side, I will update my commit accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @denis-tingaikin,

I edited my commit and changed the default behavior based on the discussion. However, some of the CI tests failed, and upon reviewing the results, it seems that some expected outputs explicitly contain the control characters that I have removed.

...
--- FAIL: TestOutput (0.00s)
    common_test.go:73: 
        	Error Trace:	/home/runner/work/sdk/sdk/pkg/networkservice/core/trace/traceconcise/common_test.go:73
        	Error:      	Not equal: 
        	            	expected: "\x1b[36m [INFO] [id:conn-1] [type:networkService] \x1b[0mserver-reque...
...

Do you have any suggestions on how to proceed?

Copy link
Member

Choose a reason for hiding this comment

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

Since we're removing control characters from logs, we should also remove them from expectations in tests.

@bszirtes bszirtes force-pushed the disable-log-coloring branch 2 times, most recently from dcef1a8 to 34f2ed5 Compare March 14, 2024 14:51
@bszirtes bszirtes changed the title Introducing an option to disable log coloring Disabling log coloring Mar 14, 2024
@bszirtes bszirtes force-pushed the disable-log-coloring branch 3 times, most recently from 7ad16d8 to 307a9c8 Compare March 18, 2024 08:56
@@ -1,4 +1,4 @@
// Copyright (c) 2021 Doc.ai and/or its affiliates.
// Copyright (c) 2024 Doc.ai and/or its affiliates.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to use a year range (2021-2024) for these cases.
Please check other modified files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I will update the files accordingly. What should I put into the header where it was
// Copyright (c) 2023 Cisco and/or its affiliates.
originally. Should I go with 2024, 2023-2024 or just use 2021-2024 everywhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

The first year should be the same.
So, you should use 2023-2024

Copy link
Member

Choose a reason for hiding this comment

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

@bszirtes also feel free to use copyright holders related to your company ;) 

For example,

// Copyright (c) 2023 Cisco and/or its affiliates.
//
// Copyright (c) 2024 My company and/or its affiliates.
//

The copyright header is correct for the linter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have made updates to the headers.

NSM issue: networkservicemesh#1594

Signed-off-by: Botond Szirtes <botond.szirtes@est.tech>
@bszirtes bszirtes changed the title Disabling log coloring Disable log coloring Mar 18, 2024
@denis-tingaikin denis-tingaikin merged commit a9e38d9 into networkservicemesh:main Mar 19, 2024
17 checks passed
nsmbot pushed a commit to networkservicemesh/cmd-csi-driver that referenced this pull request Mar 19, 2024
…k@main

PR link: networkservicemesh/sdk#1595

Commit: a9e38d9
Author: Botond Szirtes
Date: 2024-03-19 10:04:55 +0100
Message:
  - Introducing option to disable log coloring (#1595)
NSM issue: networkservicemesh/sdk#1594

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-cluster-info-k8s that referenced this pull request Mar 19, 2024
…k@main

PR link: networkservicemesh/sdk#1595

Commit: a9e38d9
Author: Botond Szirtes
Date: 2024-03-19 10:04:55 +0100
Message:
  - Introducing option to disable log coloring (#1595)
NSM issue: networkservicemesh/sdk#1594

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/sdk-kernel that referenced this pull request Mar 19, 2024
…k@main

PR link: networkservicemesh/sdk#1595

Commit: a9e38d9
Author: Botond Szirtes
Date: 2024-03-19 10:04:55 +0100
Message:
  - Introducing option to disable log coloring (#1595)
NSM issue: networkservicemesh/sdk#1594

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-dashboard-backend that referenced this pull request Mar 19, 2024
…k@main

PR link: networkservicemesh/sdk#1595

Commit: a9e38d9
Author: Botond Szirtes
Date: 2024-03-19 10:04:55 +0100
Message:
  - Introducing option to disable log coloring (#1595)
NSM issue: networkservicemesh/sdk#1594

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-map-ip-k8s that referenced this pull request Mar 19, 2024
…k@main

PR link: networkservicemesh/sdk#1595

Commit: a9e38d9
Author: Botond Szirtes
Date: 2024-03-19 10:04:55 +0100
Message:
  - Introducing option to disable log coloring (#1595)
NSM issue: networkservicemesh/sdk#1594

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-ipam-vl3 that referenced this pull request Mar 19, 2024
…k@main

PR link: networkservicemesh/sdk#1595

Commit: a9e38d9
Author: Botond Szirtes
Date: 2024-03-19 10:04:55 +0100
Message:
  - Introducing option to disable log coloring (#1595)
NSM issue: networkservicemesh/sdk#1594

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nsmgr-proxy that referenced this pull request Mar 19, 2024
…k@main

PR link: networkservicemesh/sdk#1595

Commit: a9e38d9
Author: Botond Szirtes
Date: 2024-03-19 10:04:55 +0100
Message:
  - Introducing option to disable log coloring (#1595)
NSM issue: networkservicemesh/sdk#1594

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nsc-init that referenced this pull request Mar 19, 2024
…k@main

PR link: networkservicemesh/sdk#1595

Commit: a9e38d9
Author: Botond Szirtes
Date: 2024-03-19 10:04:55 +0100
Message:
  - Introducing option to disable log coloring (#1595)
NSM issue: networkservicemesh/sdk#1594

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nse-vfio that referenced this pull request Mar 19, 2024
…k@main

PR link: networkservicemesh/sdk#1595

Commit: a9e38d9
Author: Botond Szirtes
Date: 2024-03-19 10:04:55 +0100
Message:
  - Introducing option to disable log coloring (#1595)
NSM issue: networkservicemesh/sdk#1594

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-admission-webhook-k8s that referenced this pull request Mar 19, 2024
…k@main

PR link: networkservicemesh/sdk#1595

Commit: a9e38d9
Author: Botond Szirtes
Date: 2024-03-19 10:04:55 +0100
Message:
  - Introducing option to disable log coloring (#1595)
NSM issue: networkservicemesh/sdk#1594

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-registry-proxy-dns that referenced this pull request Mar 19, 2024
…k@main

PR link: networkservicemesh/sdk#1595

Commit: a9e38d9
Author: Botond Szirtes
Date: 2024-03-19 10:04:55 +0100
Message:
  - Introducing option to disable log coloring (#1595)
NSM issue: networkservicemesh/sdk#1594

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/sdk-k8s that referenced this pull request Mar 19, 2024
…k@main

PR link: networkservicemesh/sdk#1595

Commit: a9e38d9
Author: Botond Szirtes
Date: 2024-03-19 10:04:55 +0100
Message:
  - Introducing option to disable log coloring (#1595)
NSM issue: networkservicemesh/sdk#1594

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nse-remote-vlan that referenced this pull request Mar 19, 2024
…k@main

PR link: networkservicemesh/sdk#1595

Commit: a9e38d9
Author: Botond Szirtes
Date: 2024-03-19 10:04:55 +0100
Message:
  - Introducing option to disable log coloring (#1595)
NSM issue: networkservicemesh/sdk#1594

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nsmgr that referenced this pull request Mar 19, 2024
…k@main

PR link: networkservicemesh/sdk#1595

Commit: a9e38d9
Author: Botond Szirtes
Date: 2024-03-19 10:04:55 +0100
Message:
  - Introducing option to disable log coloring (#1595)
NSM issue: networkservicemesh/sdk#1594

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-registry-memory that referenced this pull request Mar 19, 2024
…k@main

PR link: networkservicemesh/sdk#1595

Commit: a9e38d9
Author: Botond Szirtes
Date: 2024-03-19 10:04:55 +0100
Message:
  - Introducing option to disable log coloring (#1595)
NSM issue: networkservicemesh/sdk#1594

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
@bszirtes bszirtes deleted the disable-log-coloring branch March 20, 2024 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants