Skip to content

Fix glog logger conflict#1310

Merged
JulienBalestra merged 6 commits intomasterfrom
charly/flag_parse_override
Feb 21, 2018
Merged

Fix glog logger conflict#1310
JulienBalestra merged 6 commits intomasterfrom
charly/flag_parse_override

Conversation

@CharlyF
Copy link
Contributor

@CharlyF CharlyF commented Feb 20, 2018

What does this PR do?

moving glog flag parsing to the start of the agent to avoid conflict of loggers
Rely on an official fix from kubernetes/kubernetes#17162.

Motivation

Not seeing glog errors when the kubernetes client is used.
Having the correct help output when using ./agent with an incorrect command.

@CharlyF CharlyF requested a review from a team February 20, 2018 21:28
@CharlyF CharlyF requested review from a team as code owners February 20, 2018 21:28
@CharlyF CharlyF force-pushed the charly/flag_parse_override branch from 3ea15d1 to 34e09ab Compare February 21, 2018 00:09
@CharlyF CharlyF force-pushed the charly/flag_parse_override branch from 34e09ab to f1aab82 Compare February 21, 2018 00:22
---
fixes:
- |
Remove the flag.Parse() to use a fix avoiding to overide the logger.
Copy link
Contributor

Choose a reason for hiding this comment

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

override

Copy link
Member

@olivielpeau olivielpeau Feb 21, 2018

Choose a reason for hiding this comment

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

could we make this relnote more user-oriented? (i.e. link this fix to a bug in behavior that users may have experienced earlier)

Something like Fix setup of command-line options on the main `agent` command (this may need re-wording too). Let me know what you think

masci
masci previously approved these changes Feb 21, 2018
Copy link
Contributor

@masci masci left a comment

Choose a reason for hiding this comment

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

CLI works as expected

mfpierre
mfpierre previously approved these changes Feb 21, 2018
Copy link
Contributor

@mfpierre mfpierre left a comment

Choose a reason for hiding this comment

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

one nitpick but LGTM

glog was definitively not meant to be vendored 😑

JulienBalestra
JulienBalestra previously approved these changes Feb 21, 2018
Copy link
Contributor

@JulienBalestra JulienBalestra left a comment

Choose a reason for hiding this comment

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

LGTM

@masci masci added the kind/bug label Feb 21, 2018
@olivielpeau olivielpeau added this to the 6.0.0-rc.3 milestone Feb 21, 2018
@xvello xvello dismissed stale reviews from JulienBalestra, mfpierre, and masci via 672511f February 21, 2018 10:29
Copy link
Member

@olivielpeau olivielpeau left a comment

Choose a reason for hiding this comment

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

Let's merge as soon as the CI passes

@JulienBalestra JulienBalestra merged commit 3fa016c into master Feb 21, 2018
@olivielpeau olivielpeau deleted the charly/flag_parse_override branch February 21, 2018 14:50
mightyguava added a commit to mightyguava/datadog-agent that referenced this pull request Jun 27, 2022
The dgraph version of the library is abandoned and the outcaste-io one
is now the official fork. The new version removes a dependency on the
glog library, that can cause conflicts due to global initialization, see outcaste-io/ristretto#3.

The same glog library has caused issues in this repo in the past as well DataDog#1310.
mightyguava added a commit to mightyguava/datadog-agent that referenced this pull request Jun 29, 2022
The dgraph version of the library is abandoned and the outcaste-io one
is now the official fork. The new version removes a dependency on the
glog library, that can cause conflicts due to global initialization, see outcaste-io/ristretto#3.

The same glog library has caused issues in this repo in the past as well DataDog#1310.
mightyguava added a commit to mightyguava/datadog-agent that referenced this pull request Jun 29, 2022
The dgraph version of the library is abandoned and the outcaste-io one
is now the official fork. The new version removes a dependency on the
glog library, that can cause conflicts due to global initialization, see outcaste-io/ristretto#3.

The same glog library has caused issues in this repo in the past as well DataDog#1310.
mightyguava added a commit to mightyguava/datadog-agent that referenced this pull request Jun 29, 2022
The dgraph version of the library is abandoned and the outcaste-io one
is now the official fork. The new version removes a dependency on the
glog library, that can cause conflicts due to global initialization, see outcaste-io/ristretto#3.

The same glog library has caused issues in this repo in the past as well DataDog#1310.
mightyguava added a commit to mightyguava/datadog-agent that referenced this pull request Jul 11, 2022
The dgraph version of the library is abandoned and the outcaste-io one
is now the official fork. The new version removes a dependency on the
glog library, that can cause conflicts due to global initialization, see outcaste-io/ristretto#3.

The same glog library has caused issues in this repo in the past as well DataDog#1310.
mightyguava added a commit to mightyguava/datadog-agent that referenced this pull request Jul 13, 2022
The dgraph version of the library is abandoned and the outcaste-io one
is now the official fork. The new version removes a dependency on the
glog library, that can cause conflicts due to global initialization, see outcaste-io/ristretto#3.

The same glog library has caused issues in this repo in the past as well DataDog#1310.
gbbr pushed a commit that referenced this pull request Jul 15, 2022
…12559)

The dgraph version of the library is abandoned and the outcaste-io one
is now the official fork. The new version removes a dependency on the
glog library, that can cause conflicts due to global initialization, see outcaste-io/ristretto#3.

The same glog library has caused issues in this repo in the past as well #1310.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants