-
Notifications
You must be signed in to change notification settings - Fork 215
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
chore: migrate knative logger to controller-runtime injected logger #1050
chore: migrate knative logger to controller-runtime injected logger #1050
Conversation
/hold |
ping @jonathan-innis |
e9578fe
to
9a6bd6a
Compare
Pull Request Test Coverage Report for Build 9165359761Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
@fmuyassarov love this, can you include a run of with the kwok provider scale up and scale down of what these logs look like? |
Absolutely. Will update soon after having results. |
9a6bd6a
to
da6e70d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't look at everything in detail yet, but have some high-level comments and thoughts about verbosity
da6e70d
to
16fa21c
Compare
16fa21c
to
ac0688e
Compare
@Bryce-Soghigian Can comment more on the Azure provider, but the other thing to keep in mind here is that this change is going to cause the cloud providers to have to shift what logging package it's using as well, so we need to coordinate this change with a change in the cloud provider code so that cloud providers aren't blocked from upgrading to this newer version for too long. @fmuyassarov Is this also something that you would be interested in taking up? |
Sure, is there a common mailing list to broadcast the this change to the providers? I would expect community meeting is a good place to start but perhaps there are some other channels which I'm not aware of. |
c4b7d90
to
40d7cf0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really good work. Just have some comments on capitalization and nits.
768cffd
to
1c4a5ef
Compare
Signed-off-by: Feruzjon Muyassarov <[email protected]>
1c4a5ef
to
8c716ef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
Nice work!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Bryce-Soghigian, fmuyassarov, jonathan-innis, njtran The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Actually, I see a reference to the knative logger here: https://github.com/kubernetes-sigs/karpenter/blob/main/kwok/main.go#L20 can you change this too? |
772b8fa
to
b2091db
Compare
b2091db
to
172b06a
Compare
@njtran gave it the go-ahead, so we're good to merge |
/unhold |
/lgtm |
Fixes: #922
Description
Migrate knative logger to controller-runtime injected logger
After this change the logging messages look like below
How was this change tested?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.