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

Zap logger updates #874

Merged
merged 11 commits into from
May 11, 2023
Merged

Zap logger updates #874

merged 11 commits into from
May 11, 2023

Conversation

maurafortino
Copy link
Contributor

What's included:

  • Archival & Deprecation Roadmap #655
    ** this won't deprecate webpa-common fully from scytale - but will remove the logging and go-kit features from scytale
  • added zap logger to servicecfg.NewEnvironment to be used in Scytale
  • added adapter struct to act as an interface so zap logger could be used in certain functions that required the Log interface

@maurafortino maurafortino added the webpa-common archival/deprecation work related to the archival & deprecation of webpa-common label May 3, 2023
@maurafortino maurafortino self-assigned this May 3, 2023
Copy link
Contributor

@denopink denopink left a comment

Choose a reason for hiding this comment

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

I left a few suggestions.
Also, there's a few formatting notes like newlines after {} blocks like if and for and func blocks.
Looks like some webpa tests are failing

service/consul/environment.go Outdated Show resolved Hide resolved
service/consul/datacenterWatch.go Outdated Show resolved Hide resolved
service/consul/datacenterWatch.go Outdated Show resolved Hide resolved
service/consul/environment.go Outdated Show resolved Hide resolved
service/consul/environment.go Outdated Show resolved Hide resolved
service/consul/registrar.go Outdated Show resolved Hide resolved
service/consul/registrar.go Outdated Show resolved Hide resolved
service/consul/registrar.go Outdated Show resolved Hide resolved
service/consul/registrar_test.go Outdated Show resolved Hide resolved
service/zk/environment.go Outdated Show resolved Hide resolved
service/consul/environment.go Outdated Show resolved Hide resolved
adapter/adapter.go Outdated Show resolved Hide resolved
service/consul/datacenterWatch.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented May 8, 2023

Codecov Report

Merging #874 (82f2963) into main (603d223) will increase coverage by 0.00%.
The diff coverage is 78.94%.

❗ Current head 82f2963 differs from pull request most recent head 5ad7c7c. Consider uploading reports for the commit 5ad7c7c to get more accurate results

@@           Coverage Diff           @@
##             main     #874   +/-   ##
=======================================
  Coverage   81.83%   81.84%           
=======================================
  Files         138      138           
  Lines        8568     8556   -12     
=======================================
- Hits         7012     7003    -9     
+ Misses       1364     1361    -3     
  Partials      192      192           
Flag Coverage Δ
unittests 81.84% <78.94%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
service/consul/instancer.go 65.21% <40.00%> (-0.30%) ⬇️
service/consul/datacenterWatch.go 50.00% <62.50%> (ø)
service/consul/environment.go 53.57% <66.66%> (+1.05%) ⬆️
service/consul/registrar.go 100.00% <100.00%> (ø)
service/servicecfg/environment.go 100.00% <100.00%> (ø)
service/zk/environment.go 100.00% <100.00%> (ø)

Copy link
Contributor

@denopink denopink left a comment

Choose a reason for hiding this comment

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

lgtm 🍻

service/zk/environment_test.go Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented May 11, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

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

66.0% 66.0% Coverage
0.0% 0.0% Duplication

@maurafortino maurafortino merged commit 467d1b4 into main May 11, 2023
@maurafortino maurafortino deleted the zap-logger-updates branch May 11, 2023 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
webpa-common archival/deprecation work related to the archival & deprecation of webpa-common
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants