Skip to content

Remove provisioner code#2064

Merged
knative-prow-robot merged 3 commits into
knative:masterfrom
matzew:remove_provisioner_code
Oct 21, 2019
Merged

Remove provisioner code#2064
knative-prow-robot merged 3 commits into
knative:masterfrom
matzew:remove_provisioner_code

Conversation

@matzew
Copy link
Copy Markdown
Member

@matzew matzew commented Oct 21, 2019

Fixes #

Proposed Changes

  • remove provisioner logging on broker cmds

Release Note


@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Oct 21, 2019
@knative-prow-robot knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 21, 2019
@knative-prow-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matzew

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 Oct 21, 2019
@matzew matzew force-pushed the remove_provisioner_code branch from da0103d to 3c7288d Compare October 21, 2019 12:04
@matzew matzew force-pushed the remove_provisioner_code branch from 7911c0a to 037e396 Compare October 21, 2019 12:33
@matzew
Copy link
Copy Markdown
Member Author

matzew commented Oct 21, 2019

/assign @Harwayne @nachocano

Comment thread cmd/broker/filter/main.go Outdated

import (
"flag"
"knative.dev/pkg/injection"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

formatter?

Comment thread cmd/broker/filter/main.go Outdated

var env envConfig
if err := envconfig.Process("", &env); err != nil {
log.Fatal("Failed to process env var", zap.Error(err))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

logger.Fatal?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@nachocano same as in the webhook/main.go this is the log package, as we have not here initialized the knative.dev/pkg logger

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ah, sorry - I was on wrong line :-)

will update

Comment thread cmd/broker/filter/main.go Outdated
ctx, _ = injection.Default.SetupInformers(ctx, cfg)
kubeClient := kubeclient.Get(ctx)

config, err := sharedmain.GetLoggingConfig(ctx)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

there is already a cfg, and now this config. Might be clearer if we rename it to loggingCfg or loggingConfig ?

Comment thread cmd/broker/ingress/main.go Outdated

import (
"flag"
"knative.dev/pkg/logging"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

formatter

Comment thread cmd/broker/ingress/main.go Outdated
ctx, informers := injection.Default.SetupInformers(ctx, cfg)

cfg, err := sharedmain.GetConfig(*masterURL, *kubeconfig)
config, err := sharedmain.GetLoggingConfig(ctx)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same as above with the naming

Comment thread cmd/broker/ingress/main.go Outdated

var env envConfig
if err := envconfig.Process("", &env); err != nil {
log.Fatal("Failed to process env var", zap.Error(err))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

logger.Fatal

@nachocano
Copy link
Copy Markdown
Contributor

Just as a note, I think in eventing-contrib we were using this. I think I saw it in NATSS... just so that you are aware. That will have to change as well...
Or just move this old loggers to contrib and import them from there, until someone does the refactor. I did that for the message_dispatcher, message_receiver adding issues and TODOs.
It might be easier just to do the refactor in this case.

@nachocano
Copy link
Copy Markdown
Contributor

And thanks for doing this!!!

Comment thread cmd/broker/filter/main.go Outdated
if err != nil {
log.Fatal("Error loading/parsing logging configuration:", err)
}
logger, _ := logging.NewLoggerFromConfig(loggingConfig, "broker_filter")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The rest of the file expected the desugared logger, do something like:

Suggested change
logger, _ := logging.NewLoggerFromConfig(loggingConfig, "broker_filter")
sl, _ := logging.NewLoggerFromConfig(loggingConfig, "broker_filter")
logger := sl.Desugar()

Comment thread cmd/broker/ingress/main.go Outdated
log.Fatal("Error building kubeconfig", err)
log.Fatal("Error loading/parsing logging configuration:", err)
}
logger, _ := logging.NewLoggerFromConfig(loggingConfig, "broker_ingress")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same sugared thing.

Copy link
Copy Markdown
Contributor

@Harwayne Harwayne left a comment

Choose a reason for hiding this comment

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

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 21, 2019
@knative-prow-robot knative-prow-robot merged commit b0c5866 into knative:master Oct 21, 2019
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. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants