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

feat(operator): accept LogLevels for all controllers #790

Merged
merged 11 commits into from
Feb 22, 2023

Conversation

sudiptob2
Copy link
Member

@sudiptob2 sudiptob2 commented Feb 8, 2023

Fixes #724

Checklist

  • Define log levels in the values.yaml
  • Update overlay with the loglevel values
  • Adapt code to use the loglevel values from the env

@RealAnna RealAnna changed the title feat: Operator :: Accept LogLevel feat(operator): accept LogLevels for all controllers Feb 8, 2023
@codecov
Copy link

codecov bot commented Feb 8, 2023

Codecov Report

Merging #790 (1c41080) into main (2fa1a02) will increase coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #790      +/-   ##
==========================================
+ Coverage   58.59%   58.60%   +0.01%     
==========================================
  Files          97       97              
  Lines        7552     7552              
==========================================
+ Hits         4425     4426       +1     
+ Misses       2936     2935       -1     
  Partials      191      191              
Impacted Files Coverage Δ
...ptnworkloadinstance/reconcile_prepostevaluation.go 81.81% <0.00%> (-9.10%) ⬇️
...ator/controllers/lifecycle/keptntask/controller.go 54.94% <0.00%> (-3.30%) ⬇️
...lers/lifecycle/keptnworkloadinstance/controller.go 82.95% <0.00%> (+1.79%) ⬆️
...ptnworkloadinstance/reconcile_prepostdeployment.go 90.90% <0.00%> (+9.09%) ⬆️
Flag Coverage Δ
component-tests 42.06% <ø> (+0.40%) ⬆️
keptn-lifecycle-operator 54.67% <ø> (ø)
klt-cert-manager 67.50% <ø> (ø)
scheduler 21.17% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@sudiptob2 sudiptob2 force-pushed the feat/724/operator-log-level branch from 8d1359f to d48a428 Compare February 15, 2023 03:15
@sudiptob2
Copy link
Member Author

@RealAnna
I am trying to understand the logger setup in KLT. Zapr logger is set up here as follows

	opts := zap.Options{
		Development: true,
	}
	opts.BindFlags(flag.CommandLine)

At this time we can define the log level as

	opts := zap.Options{
		Development: true,
                Level: env.SomeControllerLogLevel
	}

But definitely, we want to configure the different log levels for different controllers. In this case what I think we can do is, patch the log level after the definition of each Reconciler. For example, we can add the following here.

taskReconciler.Log.V(env.SomeControllerLogLevel)

However, I am unsure about the syntax, because zapr takes negative values, but the expected value in Reconciler.Log.V() is positive.

I would appreciate it tremendously if you could guide me in this case.

@sudiptob2 sudiptob2 force-pushed the feat/724/operator-log-level branch from d48a428 to 3f419d0 Compare February 15, 2023 18:09
@sudiptob2 sudiptob2 marked this pull request as ready for review February 15, 2023 18:26
@sudiptob2 sudiptob2 marked this pull request as draft February 16, 2023 07:45
@sudiptob2 sudiptob2 force-pushed the feat/724/operator-log-level branch from 05be94e to a494d86 Compare February 16, 2023 10:38
@sudiptob2 sudiptob2 marked this pull request as ready for review February 16, 2023 10:39
RealAnna
RealAnna previously approved these changes Feb 16, 2023
@netlify
Copy link

netlify bot commented Feb 22, 2023

Deploy Preview for keptn-lifecyle-testing ready!

Name Link
🔨 Latest commit 1c41080
🔍 Latest deploy log https://app.netlify.com/sites/keptn-lifecyle-testing/deploys/63f5e3146bb74500081905e9
😎 Deploy Preview https://deploy-preview-790--keptn-lifecyle-testing.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

thisthat
thisthat previously approved these changes Feb 22, 2023
Copy link
Member

@thisthat thisthat left a comment

Choose a reason for hiding this comment

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

Awesome work 👍 Thanks @sudiptob2

Signed-off-by: Giovanni Liva <[email protected]>
@sonarcloud
Copy link

sonarcloud bot commented Feb 22, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@RealAnna RealAnna merged commit d175486 into keptn:main Feb 22, 2023
@keptn-bot keptn-bot mentioned this pull request Feb 22, 2023
Amishakumari544 pushed a commit to Amishakumari544/lifecycle-toolkit that referenced this pull request Feb 27, 2023
Signed-off-by: Sudipto Baral <[email protected]>
Signed-off-by: Giovanni Liva <[email protected]>
Co-authored-by: Giovanni Liva <[email protected]>
Signed-off-by: Amishakumari544 <[email protected]>
aepfli pushed a commit to aepfli/lifecycle-toolkit that referenced this pull request Mar 1, 2023
Signed-off-by: Sudipto Baral <[email protected]>
Signed-off-by: Giovanni Liva <[email protected]>
Co-authored-by: Giovanni Liva <[email protected]>
@keptn-bot keptn-bot mentioned this pull request Mar 16, 2023
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.

Operator :: Accept LogLevel
3 participants