Skip to content

Fix the sugared logger usage#3345

Merged
knative-prow-robot merged 9 commits into
knative:masterfrom
vagababov:holding-hands-with-error
Mar 5, 2019
Merged

Fix the sugared logger usage#3345
knative-prow-robot merged 9 commits into
knative:masterfrom
vagababov:holding-hands-with-error

Conversation

@vagababov
Copy link
Copy Markdown
Contributor

While debugging tests this got so annoying,
since the output is useless, so I had to go
and clean this stuff up.

/lint

Fixes #

Proposed Changes

  • fix sugared logger usage.

/cc @dgerd
/assign @dgerd

Dan, I believe, you had an issue for? Cursory search did not return anything.

While debugging tests this got so annoying,
since the output is useless, so I had to go
and clean this stuff up.
@knative-prow-robot knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 28, 2019
Copy link
Copy Markdown
Contributor

@markusthoemmes markusthoemmes left a comment

Choose a reason for hiding this comment

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

One nit, LGTM overall though.

/approve

Comment thread pkg/reconciler/v1alpha1/service/service.go Outdated
Comment thread pkg/reconciler/stats.go Outdated
Comment thread pkg/autoscaler/statserver/server_test.go Outdated
Comment thread pkg/reconciler/v1alpha1/revision/revision.go Outdated
Comment thread pkg/reconciler/v1alpha1/service/service.go Outdated
Comment thread pkg/reconciler/v1alpha1/service/service.go Outdated
@vagababov
Copy link
Copy Markdown
Contributor Author

All done.

logger.Errorf("Failed to reconcile Service: %q failed to reconcile Configuration: %q; %v", service.Name, configName, zap.Error(err))
logger.Errorw(
fmt.Sprintf("Failed to reconcile Service: %q failed to reconcile Configuration: %q",
service.Name, configName), err)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

One more fix. :)

Suggested change
service.Name, configName), err)
service.Name, configName), zap.Error(err))

@vagababov
Copy link
Copy Markdown
Contributor Author

/test pull-knative-serving-integration-tests
unrealted

@vagababov
Copy link
Copy Markdown
Contributor Author

/test pull-knative-serving-integration-tests

@dgerd
Copy link
Copy Markdown

dgerd commented Mar 5, 2019

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 5, 2019
@knative-metrics-robot
Copy link
Copy Markdown

The following is the coverage report on pkg/.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/autoscaler/statserver/server.go 79.2% 80.8% 1.5

Copy link
Copy Markdown
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@knative-prow-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dgerd, markusthoemmes, mattmoor, vagababov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 5, 2019
@knative-prow-robot knative-prow-robot merged commit 7554360 into knative:master Mar 5, 2019
@vagababov vagababov deleted the holding-hands-with-error branch March 21, 2019 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants