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

Update from Sirupsen -> sirupsen #1573

Merged
merged 2 commits into from
Sep 22, 2017
Merged

Update from Sirupsen -> sirupsen #1573

merged 2 commits into from
Sep 22, 2017

Conversation

aanm
Copy link
Member

@aanm aanm commented Sep 22, 2017

No description provided.

@aanm aanm added kind/bug This is a bug in the Cilium logic. kind/enhancement This would improve or streamline existing functionality. labels Sep 22, 2017
@aanm aanm requested a review from ashwinp September 22, 2017 10:17
@aanm aanm added the wip label Sep 22, 2017
Copy link
Member

@nebril nebril left a comment

Choose a reason for hiding this comment

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

LGMT, but I think that we shouldn't leave code in comments, but track the fix in separate issue/retrieve the old version to fix from git.

@@ -242,24 +241,24 @@ func setupFluentD(logOpts map[string]string, debug bool) {
// setupLogstash sets up and configures Logstash with the provided options in
// logOpts. If some options are not provided, sensible defaults are used.
/// TODO fix me later - needs to be tested with a working logstash setup.
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove it and grab it from git later/track it in a separate issue?

Copy link
Member

Choose a reason for hiding this comment

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

What I've started doing is to file a GH issues and refer to it from the comment in the code ie in the form:

// GH-1231: text

Also update all other dependencies that were using Sirupsen instead of
sirupsen

Signed-off-by: André Martins <andre@cilium.io>
Signed-off-by: André Martins <andre@cilium.io>
@aanm aanm force-pushed the update-to-sirupsen-log branch from ec10001 to 7053218 Compare September 22, 2017 15:18
@aanm aanm added pending-review and removed wip labels Sep 22, 2017
@aanm
Copy link
Member Author

aanm commented Sep 22, 2017

@tgraf @nebril I've left the code commented and wrote with the GH format. If we drop support for logstash we can remove the function later on. TBD in #1578

Copy link
Contributor

@ashwinp ashwinp left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -25,6 +25,10 @@ Documentation
-------------
* Add here

Other
-----
* Update Sirupsen/logrus to sirupsen/logrus (1573_)
Copy link
Member

Choose a reason for hiding this comment

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

👍 Thank you!

@tgraf tgraf merged commit 0df4648 into master Sep 22, 2017
@tgraf tgraf deleted the update-to-sirupsen-log branch September 22, 2017 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug This is a bug in the Cilium logic. kind/enhancement This would improve or streamline existing functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants