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

Remove logging from function and structs. #582

Merged
merged 7 commits into from
Dec 14, 2017
Merged

Remove logging from function and structs. #582

merged 7 commits into from
Dec 14, 2017

Conversation

shawn-hurley
Copy link
Contributor

@shawn-hurley shawn-hurley commented Dec 7, 2017 •
edited

Describe what this PR does and why we need it:
Removes logging from the function signatures and structures
Changes proposed in this pull request

  • create a utility/logging package to manage the logging.
  • adds log var to each package that needs it that is package accessible.
  • removes log from functions and structs

Most of the fixes are in util/logging/logger.go. All the other changes are setting up the logger per package and removing the log from structures and functions

Which issue this PR fixes (This will close that issue when PR gets merged)
fixes #270

@shawn-hurley shawn-hurley added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. feature needs-review labels Dec 7, 2017
@openshift-ci-robot openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 7, 2017
@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 7, 2017
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 0521d0e on shawn-hurley:logging-impl into ** on openshift:master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 0521d0e on shawn-hurley:logging-impl into ** on openshift:master**.

@jmrodri jmrodri changed the title [WIP] Logging changes to remove logging from function and structs. [WIP] Remove logging from function and structs. Dec 7, 2017
jmrodri
jmrodri reviewed Dec 7, 2017
View reviewed changes
pkg/apb/types.go Outdated
"github.com/openshift/ansible-service-broker/pkg/config"
utillogging "github.com/openshift/ansible-service-broker/pkg/util/logging"
Copy link
Contributor

Choose a reason for hiding this comment

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

After reading it a few times, how about 'logutilinstead ofutillogging`? :)

jmrodri
jmrodri reviewed Dec 7, 2017
View reviewed changes
pkg/app/app.go Outdated
fmt.Printf("%#v", app.config)

if app.log, err = NewLog(app.config); err != nil {
c := utillogging.LogConfig{
Copy link
Contributor

Choose a reason for hiding this comment

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

logutil instead of utillogging

jmrodri
jmrodri suggested changes Dec 7, 2017
View reviewed changes
Copy link
Contributor

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

looks great. On minor nit is rename utillogging to logutil unless of course goimports picked the name for you then it's probably ok.

pkg/util/logging/logger.go Outdated

//InitilizeLog - initilize the logging utility
Copy link
Contributor

Choose a reason for hiding this comment

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

space after the //

pkg/util/logging/logger.go Outdated

//InitilizeLog - initilize the logging utility
func InitilizeLog(config LogConfig) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

TYPO: InitilizeLog -> InitializeLog

pkg/auth/auth.go Outdated
)

var log = utillogging.NewLog()
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah so each package does this call. I was searching for where the hell the logger was set. Found it.

jmrodri
jmrodri reviewed Dec 7, 2017
View reviewed changes
pkg/util/logging/logger.go
@@ -22,7 +22,6 @@ import (
"os"

logging "github.com/op/go-logging"
"github.com/openshift/ansible-service-broker/pkg/config"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

The package of this file is package app yet it lives in util/logging which is the right package?

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling a2de11a on shawn-hurley:logging-impl into ** on openshift:master**.

@shawn-hurley shawn-hurley changed the title [WIP] Remove logging from function and structs. Remove logging from function and structs. Dec 8, 2017
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 8, 2017
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling c91f124 on shawn-hurley:logging-impl into ** on openshift:master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling fef1f08 on shawn-hurley:logging-impl into ** on openshift:master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling e3fcf9a on shawn-hurley:logging-impl into ** on openshift:master**.

@rthallisey
Copy link
Contributor

👍 pending Zeus' comment

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling e1c4862 on shawn-hurley:logging-impl into ** on openshift:master**.

eriknelson
eriknelson approved these changes Dec 14, 2017
View reviewed changes
Copy link
Contributor

@eriknelson eriknelson left a comment

Choose a reason for hiding this comment

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

Swore I reviewed this but I must have never submitted or something. Really happy to see the logs removed from the credentials.

Something feels weird about package level globals to me but I know it's a pretty standard go pattern.

djzager
djzager approved these changes Dec 14, 2017
View reviewed changes
Copy link
Member

@djzager djzager left a comment

Choose a reason for hiding this comment

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

LGTM

pkg/apb/bind.go
parameters *Parameters,
log *logging.Logger,
) (string, *ExtractedCredentials, error) {
func Bind(instance *ServiceInstance,
Copy link
Member

Choose a reason for hiding this comment

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

Gross 😃

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling df55e45 on shawn-hurley:logging-impl into ** on openshift:master**.

@shawn-hurley shawn-hurley merged commit b5aa9f5 into openshift:master Dec 14, 2017
jianzhangbjz pushed a commit to jianzhangbjz/ansible-service-broker that referenced this pull request May 17, 2018
@shawn-hurley
Remove logging from function and structs. (openshift#582)
6b98745 
* logging changes to remove logging from function and structs.
* fixing the testing of logging.
* adding test cases and addressing comments.
* fixing lines that should be broken due to length
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmrodri jmrodri jmrodri approved these changes

@eriknelson eriknelson eriknelson approved these changes

@djzager djzager djzager approved these changes

@rthallisey rthallisey rthallisey approved these changes

Assignees
No one assigned
Labels
feature needs-review size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Warn about unknown keys found in config.
7 participants
@shawn-hurley @coveralls @rthallisey @jmrodri @eriknelson @djzager @openshift-ci-robot