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.

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

Initial structure of log repo #272

Merged
merged 9 commits into from
Sep 11, 2019
Merged

Initial structure of log repo #272

merged 9 commits into from
Sep 11, 2019

Conversation

SupriyaKasten
Copy link
Contributor

@SupriyaKasten SupriyaKasten commented Sep 10, 2019

Change Overview

Make logs more descriptive and easy to use & read

This task will be divided in small PRs and merged
This PR is the initial structure of the log repo

Pull request type

Please check the type of change your PR introduces:

  • Work in Progress
  • Refactoring (no functional changes, no api changes)
  • Trival/Minor
  • Bugfix
  • Feature
  • Documentation

@SupriyaKasten SupriyaKasten marked this pull request as ready for review September 11, 2019 00:48
@SupriyaKasten SupriyaKasten changed the title WIP: log repo Initial design of log repo Sep 11, 2019
@SupriyaKasten SupriyaKasten changed the title Initial design of log repo Initial structure of log repo Sep 11, 2019
pkg/log/log.go Outdated Show resolved Hide resolved
Copy link
Contributor

@tdmanv tdmanv left a comment

Choose a reason for hiding this comment

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

Almost there!

pkg/log/log.go Outdated Show resolved Hide resolved
pkg/log/log.go Outdated Show resolved Hide resolved
pkg/log/log.go Outdated
type logLevel string

const (
logTypeInfo logLevel = "Info"
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than use string literals, please use the constants from logrus

pkg/log/log.go Outdated Show resolved Hide resolved
pkg/log/log.go Outdated Show resolved Hide resolved
pkg/log/log.go Outdated Show resolved Hide resolved
pkg/log/log.go Outdated Show resolved Hide resolved
pkg/log/log.go Outdated Show resolved Hide resolved
pkg/log/printer.go Outdated Show resolved Hide resolved
pkg/log/printer.go Outdated Show resolved Hide resolved
pkg/log/printer.go Outdated Show resolved Hide resolved
pkg/log/log.go Outdated Show resolved Hide resolved
)

type logger struct {
level Level
Copy link
Contributor

Choose a reason for hiding this comment

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

We can revise the implementation later. For now, this interface helps modifying / adding the call points.

@SupriyaKasten
Copy link
Contributor Author

@julio-lopez I did not commit your suggestions directly since I wanted to make some more changes. Pushed a PR with your suggestions, Ready for another review!

pkg/log/log.go Outdated
@@ -47,22 +47,24 @@ func WithContext(ctx context.Context) {
}

func (l *logger) Print(msg string) {
var message interface{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it was failing before with: pkg/log/log.go:52:14: cannot use msg (type string) as type []interface {} in argument to logrus.Info

Copy link
Contributor

Choose a reason for hiding this comment

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

OK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I think I know where I went wrong! pushing an updated PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the confusion @julio-lopez, I was calling logrus.Info(msg...) instead if logrus.Info(msg). Fixing it now

@julio-lopez julio-lopez self-requested a review September 11, 2019 18:44
@SupriyaKasten SupriyaKasten merged commit b32b53c into master Sep 11, 2019
@SupriyaKasten SupriyaKasten deleted the controller-log branch September 11, 2019 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants