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

refactor(xds) rename logger to have consistent naming style #2375

Merged
merged 1 commit into from
Jul 25, 2021
Merged

refactor(xds) rename logger to have consistent naming style #2375

merged 1 commit into from
Jul 25, 2021

Conversation

burntcarrot
Copy link
Contributor

@burntcarrot burntcarrot commented Jul 15, 2021 •
edited
Loading

Signed-off-by: Aadhav Vignesh aadhav.n1@gmail.com

Summary

rename logger names to be consistent in the xDS server package

Full changelog

  • [Implement ...]
  • [Fix ...]

Issues resolved

Updates #2313

Documentation

Testing

  • Unit tests
  • E2E tests
  • Manual testing on Universal
  • Manual testing on Kubernetes

Backwards compatibility

  • Add backport-to-stable label if the code is backwards compatible. Otherwise, list breaking changes.

@burntcarrot
refactor(xds) rename logger to have consistent naming style
3647840 
Signed-off-by: Aadhav Vignesh <aadhav.n1@gmail.com>
@burntcarrot burntcarrot requested a review from a team as a code owner July 15, 2021 07:14
@CLAassistant
Copy link

CLAassistant commented Jul 15, 2021 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@codecov-commenter
Copy link

Codecov Report

Merging #2375 (3647840) into master (b0c72d1) will increase coverage by 0.40%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2375      +/-   ##
==========================================
+ Coverage   52.13%   52.54%   +0.40%     
==========================================
  Files         874      878       +4     
  Lines       47826    47867      +41     
==========================================
+ Hits        24936    25151     +215     
+ Misses      20865    20664     -201     
- Partials     2025     2052      +27
Impacted Files Coverage Δ
...g/xds/server/callbacks/dataplane_status_tracker.go 97.64% <ø> (ø)
pkg/core/resources/model/rest/resource.go 67.94% <0.00%> (-1.29%) ⬇️
...kg/plugins/resources/k8s/native/pkg/test/within.go 100.00% <0.00%> (ø)
api/internal/util/proto/types.go 100.00% <0.00%> (ø)
...k8s/native/controllers/proxytemplate_controller.go 0.00% <0.00%> (ø)
api/internal/util/proto/proto.go 70.00% <0.00%> (ø)
api/mesh/v1alpha1/dataplane_helpers.go 80.16% <0.00%> (+1.68%) ⬆️
pkg/core/resources/manager/cache.go 84.41% <0.00%> (+2.59%) ⬆️
pkg/plugins/resources/memory/store.go 82.06% <0.00%> (+4.34%) ⬆️
api/mesh/v1alpha1/dataplane_insight_helpers.go 83.33% <0.00%> (+5.20%) ⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b0c72d1...3647840. Read the comment docs.

@jpeach
Copy link
Contributor

jpeach commented Jul 20, 2021

@burntcarrot I pinged slack to see if anyone depends on these names. No-one has mentioned any concerns so I'd say we are good here.

Could you please update the Github tags to say "Updates #2313" rather than "fixes"? That way, the issue will remain open and we can make more PRs against it.

@burntcarrot
Copy link
Contributor Author

Yeah, I have been keeping an eye on the Slack from quite a while, and I guess we're safe to move forward.

@burntcarrot
Copy link
Contributor Author

burntcarrot commented Jul 21, 2021 •
edited
Loading

I've updated it. In case anyone raises some concerns regarding the logger names in the future, we can safely issue more fixes. This would hopefully prevent any services running dependent on the logger names subject to breaking changes.

@jpeach PR is ready for review, could you please take some time to review it?

@burntcarrot
Copy link
Contributor Author

Just out of curiosity, are the changes meant to be a part of the next release, or are you trying to push the changes to be a part of a later release, allowing room for more people to raise concerns?

jpeach
jpeach approved these changes Jul 21, 2021
View reviewed changes
Copy link
Contributor

@jpeach jpeach left a comment

Choose a reason for hiding this comment

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

Nice.

@jpeach
Copy link
Contributor

jpeach commented Jul 21, 2021

Just out of curiosity, are the changes meant to be a part of the next release, or are you trying to push the changes to be a part of a later release, allowing room for more people to raise concerns?

I don't have a strong opinion. I'm inclined to backport this to the 1.2 branch, but I'll ask around the maintainers.

ccfishk
ccfishk reviewed Jul 23, 2021
View reviewed changes
pkg/xds/server/callbacks/dataplane_status_tracker.go
@@ -16,7 +16,7 @@ import (
util_xds "github.com/kumahq/kuma/pkg/util/xds"
)

var statusTrackerLog = core.Log.WithName("xds").WithName("statusTracker")
var statusTrackerLog = core.Log.WithName("xds").WithName("status-tracker")
Copy link

Choose a reason for hiding this comment

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

lgtm.

ccfishk
ccfishk approved these changes Jul 23, 2021
View reviewed changes
Copy link

@ccfishk ccfishk left a comment

Choose a reason for hiding this comment

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

consistent naming is good.

@burntcarrot
Copy link
Contributor Author

Any updates? I guess it's better to merge it now, in order to make it available for the next release.

@jpeach jpeach merged commit bdd4b2a into kumahq:master Jul 25, 2021
mergify bot pushed a commit that referenced this pull request Jul 25, 2021
@burntcarrot
refactor(xds) rename logger to have consistent naming style (#2375)
4d378ed 
Signed-off-by: Aadhav Vignesh <aadhav.n1@gmail.com>
(cherry picked from commit bdd4b2a)
@mergify mergify bot mentioned this pull request Jul 25, 2021
Merged
jpeach pushed a commit that referenced this pull request Jul 26, 2021
@burntcarrot @jpeach
refactor(xds) rename logger to have consistent naming style (#2375)
583c2c6 
Signed-off-by: Aadhav Vignesh <aadhav.n1@gmail.com>
(cherry picked from commit bdd4b2a)
jpeach pushed a commit that referenced this pull request Jul 27, 2021
@mergify @burntcarrot
refactor(xds) rename logger to have consistent naming style (#2375) (#…
e25b059 
…2423)

Signed-off-by: Aadhav Vignesh <aadhav.n1@gmail.com>
(cherry picked from commit bdd4b2a)

Co-authored-by: Aadhav Vignesh <aadhav.n1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ccfishk ccfishk ccfishk approved these changes

@jpeach jpeach jpeach approved these changes

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

Successfully merging this pull request may close these issues.
5 participants
@burntcarrot @CLAassistant @codecov-commenter @jpeach @ccfishk