Skip to content
This repository has been archived by the owner on May 6, 2022. It is now read-only.

fix glog logging #1204

Merged
merged 1 commit into from
Sep 7, 2017
Merged

Conversation

jboyd01
Copy link
Contributor

@jboyd01 jboyd01 commented Sep 7, 2017

force go flag initialization to satisfy glog that cmd line parameters have been initialized. Fixes #1187

"ERROR: logging before flag.Parse" is prefacing all log entries for Catalog Controller and API servers.

Workaround pulled from kubernetes/kubernetes#17162.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 7, 2017
we want to pick up the prior version to fix issue 1187.
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 7, 2017
@jboyd01
Copy link
Contributor Author

jboyd01 commented Sep 7, 2017

All set for review @pmorie. I reverted the code change and as we discussed, updated glide.yaml with the current sha1 from glide.lock for every package that lacked a version and changed glog to point to the previous version. I verified both API and Controller servers are logging correctly.

@MHBauer
Copy link
Contributor

MHBauer commented Sep 7, 2017

Why should we prefer this over the workaround?

@pmorie pmorie added the LGTM1 label Sep 7, 2017
@pmorie
Copy link
Contributor

pmorie commented Sep 7, 2017

@MHBauer i think anything that adds specificity to the glide yaml is beneficial. I would also like to not be on a version of glog that main k8s is on, and the workaround is really silly (since we don't use flag)

@MHBauer
Copy link
Contributor

MHBauer commented Sep 7, 2017

not be on a version of glog that main k8s is on

What? Why?

@pmorie
Copy link
Contributor

pmorie commented Sep 7, 2017

@MHBauer i don't want to hit some random bug on a newer version of dependency than main k8s is. That's not the issue here, but that's my general feeling on the subject.

@MHBauer
Copy link
Contributor

MHBauer commented Sep 7, 2017

Not being on a newer version of glog is okay.

I'm fine with glide cleanup in general.

I do not understand why we would we explicitly not sync with the same version of glog upstream-k8s has.

EDIT: dash for clarity.

Copy link
Contributor

@arschles arschles left a comment

Choose a reason for hiding this comment

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

Thanks @jboyd01

LGTM

@arschles arschles added the LGTM2 label Sep 7, 2017
@pmorie
Copy link
Contributor

pmorie commented Sep 7, 2017

I do not understand why we would we explicitly not sync with the same version of glog upstream k8s has.

I think we might have a communication mismatch. This PR pins us to the version of glog k8s is on.

@MHBauer
Copy link
Contributor

MHBauer commented Sep 7, 2017

@pmorie okay, fine with me. I'm not sure how I could interpret "I would also like to not be on a version of glog that main k8s is on" as meaning we want the same version of k8s, but okay.

EDIT: indication that emphasis is mine.

Copy link
Contributor

@MHBauer MHBauer left a comment

Choose a reason for hiding this comment

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

LGTM

didn't check that the version matching is true, but assuming good faith that it is.

@MHBauer MHBauer added the LGTM3 label Sep 7, 2017
@MHBauer MHBauer merged commit f9dbd4e into kubernetes-retired:master Sep 7, 2017
@pmorie
Copy link
Contributor

pmorie commented Sep 7, 2017

k8s is on

Oops, I meant "isn't on"

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. LGTM1 LGTM2 LGTM3 size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants