Remove vestiges of glog command-line flags. by geeknoid · Pull Request #3759 · istio/istio

geeknoid · 2018-02-24T17:30:51Z

No description provided.

ldemailly · 2018-02-24T17:36:10Z

/test e2e-suite-rbac-auth

ldemailly

nice, qq about left over -v x:

ldemailly · 2018-02-24T17:39:04Z

.circleci/config.yml

is the leftover -v 2 doing anything now ? will it break ?

It wasn't clear that all the -v X flags were indeed for glog, so I submitted as-is. Clearly, they are indeed for glog. I'll update and try again.

it might be safer to just absorb (or even better translate into level == debug ) the -v in case some of our customers have their own templates for installing istio ?

(@costinm thoughts on backward compatibility needs for this ?)

ps: you missed this line's "-v 2" - and should those be replaced by -loglevel debug?

ldemailly · 2018-02-24T17:40:57Z

circle beat me to it almost:

flag provided but not defined: -v

who needs reviews when we have good tests ;-)

ldemailly · 2018-02-24T18:04:36Z

tests/e2e/tests/pilot/testdata/pilot.yaml.tmpl

replace by the equivalent with the new logger?

ldemailly · 2018-02-24T18:52:55Z

I0224 18:25:56.587] po/istio-ingress-68bfbc686d-k8s4v 0/1 Error 1 5s 10.28.26.132 gke-istio-e2e-rbac-rotat-default-pool-708ea9ff-s5bx

it appears the ingress yaml /... is somehow not happy

ldemailly · 2018-02-24T20:15:18Z

can you add 1 test at least that the glog shim works
I filled https://github.com/istio/glog/issues/1 but it'll probably get lost there

Phase 1 of #3759

ldemailly · 2018-02-24T20:43:43Z

Please (cd vendor; git checkout 5392831d46eb36cb168e818f1872eae39d9660d5 ) to further test
the sha on master is gone for now/until this works to not block other changes

ldemailly · 2018-02-25T23:19:13Z

can you rebase and retest ?

geeknoid · 2018-02-26T14:41:26Z

/hold

istio-merge-robot · 2018-02-27T08:04:20Z

@geeknoid PR needs rebase

costinm · 2018-02-27T23:55:43Z

2 more issies:

we use scratch images, so there is no /tmp or good way to get the logs. Can we make sure that stderr is the default ?
at least for helm, we need a way to configure the debug level. Not sure what's the equivalent of '-v 2' in the new log - but should be included in the templates.

ldemailly · 2018-02-27T23:56:19Z

/test e2e-suite-rbac-auth

ldemailly · 2018-02-28T00:38:08Z

I0228 00:00:58.753] istio-ca-847fcd677-gmfsz 0/1 ImagePullBackOff 0 2m

ldemailly · 2018-02-28T00:44:15Z

ca error is odd but looks unrelated
so I merged istio/old_vendor-istio_repo#13
@geeknoid can you quickly update this PR's vendor/ to the merged sha from master there

git diff vendor should show

+3888d5d41b389c70808311cd43cd5ae9bf94ee2a

so we can merge this one so istio/istio doesn't stay behind vendor too long (in case someone else starts a new PR with vendor changes...)

ldemailly · 2018-02-28T07:52:38Z

ping^

geeknoid · 2018-02-28T13:37:48Z

To answer Laurent's question, you get this on the command-line when using a deprectaed option:

Flag --v has been deprecated, please use --log_output_level instead

To address Costin's issue, this PR doesn't change anything in the behavior of the system, it merely removes useless command-line options from our help messages and from our command-lines. So these concerns should be filed as separate issues.

istio-testing · 2018-02-28T14:08:13Z

@geeknoid: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
prow/e2e-suite-rbac-auth.sh	340a788f20bd3b8889f2380138808b8d65ebe821	link	`/test e2e-suite-rbac-auth`

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

geeknoid · 2018-02-28T14:17:26Z

/test e2e-simple

geeknoid · 2018-02-28T14:30:22Z

/unhold

geeknoid · 2018-02-28T14:31:11Z

All green and happy. Just needs approval.

costinm · 2018-02-28T15:31:31Z

/lgtm

Will file a bug for flag defaults.

ldemailly · 2018-02-28T15:56:46Z

/lgtm

istio-merge-robot · 2018-02-28T15:57:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: costinm, ldemailly

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [costinm,ldemailly]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

ldemailly · 2018-02-28T15:59:27Z

/hold cancel

istio-merge-robot · 2018-02-28T16:00:24Z

/test all [submit-queue is verifying that this PR is safe to merge]

istio-merge-robot · 2018-02-28T16:23:14Z

Automatic merge from submit-queue.

douglas-reid · 2018-03-01T06:13:13Z

mixer/doc/running-local-mixer.md

 The following command sends a `report` request to Mixer.
 ```shell
-bazel-bin/mixer/cmd/mixc/mixc report -v 2 --string_attributes destination.service=abc.ns.svc.cluster.local,source.name=myservice,target.port=8080 --stringmap_attributes "request.headers=clnt:abc;source:abcd,destination.labels=app:ratings,source.labels=version:v2"  --int64_attributes response.duration=2003,response.size=1024 --timestamp_attributes  request.time="2017-07-04T00:01:10Z" --bytes_attributes source.ip=c0:0:0:2
+bazel-bin/mixer/cmd/mixc/mixc report --string_attributes destination.service=abc.ns.svc.cluster.local,source.name=myservice,target.port=8080 --stringmap_attributes "request.headers=clnt:abc;source:abcd,destination.labels=app:ratings,source.labels=version:v2"  --int64_attributes response.duration=2003,response.size=1024 --timestamp_attributes  request.time="2017-07-04T00:01:10Z" --bytes_attributes source.ip=c0:0:0:2


when we updated this, we should have removed references to bazel ;). Follow-on PR to remove bazel-bin ?

ldemailly · 2018-03-01T06:54:24Z

did you run "make depend.status" before the PRs ? because it's failing on circleci
https://circleci.com/gh/istio/istio/39019

make depend.status
No error means your Gopkg.* are in sync and ok with vendor/
dep status -dot > /go/out/linux_amd64/release/dep.dot
Lock inputs-digest mismatch. This happens when Gopkg.toml is modified.
Run `dep ensure` to regenerate the inputs-digest.
input-digest mismatch
Makefile:234: recipe for target 'depend.status' failed
make: *** [depend.status] Error 1

and for me too when I try to fix it:

Solving failure: No versions of gopkg.in/russross/blackfriday.v2 met constraints:
	v2.0.0: Could not introduce gopkg.in/russross/blackfriday.v2@v2.0.0, as its subpackage gopkg.in/russross/blackfriday.v2 does not contain usable Go code (*build.NoGoError).. (Package is required by github.com/istio/tools@master.)
	v2: Could not introduce gopkg.in/russross/blackfriday.v2@v2, as it is not allowed by constraint ^2.0.0 from project github.com/istio/tools.
	v2-commonmark-testsuite: Could not introduce gopkg.in/russross/blackfriday.v2@v2-commonmark-testsuite, as it is not allowed by constraint ^2.0.0 from project github.com/istio/tools.
	v2-issue423-crlf: Could not introduce gopkg.in/russross/blackfriday.v2@v2-issue423-crlf, as it is not allowed by constraint ^2.0.0 from project github.com/istio/tools.

geeknoid requested review from costinm and ldemailly as code owners February 24, 2018 17:30

geeknoid requested review from a team February 24, 2018 17:30

googlebot added the cla: yes label Feb 24, 2018

ldemailly reviewed Feb 24, 2018

View reviewed changes

geeknoid requested a review from a team February 24, 2018 17:58

ldemailly reviewed Feb 24, 2018

View reviewed changes

tests/e2e/tests/pilot/testdata/pilot.yaml.tmpl Outdated

Copy link
Copy Markdown

Member

ldemailly Feb 24, 2018

Choose a reason for hiding this comment

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

replace by the equivalent with the new logger?

This was referenced Feb 24, 2018

Replace real glog with our custom shim to our logging story. istio/old_vendor-istio_repo#7

Merged

Remove leftover uses of glog from our code base. #3761

Closed

ldemailly added a commit that referenced this pull request Feb 24, 2018

Trying latest vendor/glog she without any other change

af9faf7

Phase 1 of #3759

ldemailly mentioned this pull request Feb 24, 2018

Trying latest vendor/glog shim without any other change #3762

Closed

geeknoid requested review from a team and nmittler February 24, 2018 22:12

istio-testing added the do-not-merge/hold Block automatic merging of a PR. label Feb 26, 2018

istio-merge-robot added the needs-rebase Indicates a PR needs to be rebased before being merged label Feb 27, 2018

Remove vestiges of glog command-line flags.

1bdcf72

costinm approved these changes Feb 28, 2018

View reviewed changes

istio-testing assigned costinm Feb 28, 2018

istio-testing added the lgtm label Feb 28, 2018

istio-merge-robot added the approved label Feb 28, 2018

istio-testing assigned ldemailly Feb 28, 2018

istio-testing removed the do-not-merge/hold Block automatic merging of a PR. label Feb 28, 2018

istio-merge-robot merged commit c6166bf into istio:master Feb 28, 2018

douglas-reid reviewed Mar 1, 2018

View reviewed changes

yangminzhu mentioned this pull request Mar 3, 2018

Change the Infof to Debugf in KindMap() to fix #3835. #3925

Merged

myidpt mentioned this pull request Mar 19, 2018

istio-auth-node-agent service cannot be started #4357

Closed

Conversation

geeknoid commented Feb 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldemailly commented Feb 24, 2018

Uh oh!

ldemailly left a comment

Choose a reason for hiding this comment

Uh oh!

ldemailly Feb 24, 2018

Choose a reason for hiding this comment

Uh oh!

geeknoid Feb 24, 2018

Choose a reason for hiding this comment

Uh oh!

ldemailly Feb 24, 2018

Choose a reason for hiding this comment

Uh oh!

ldemailly Feb 24, 2018

Choose a reason for hiding this comment

Uh oh!

ldemailly commented Feb 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldemailly Feb 24, 2018

Choose a reason for hiding this comment

Uh oh!

ldemailly commented Feb 24, 2018

Uh oh!

ldemailly commented Feb 24, 2018

Uh oh!

ldemailly commented Feb 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldemailly commented Feb 25, 2018

Uh oh!

geeknoid commented Feb 26, 2018

Uh oh!

istio-merge-robot commented Feb 27, 2018

Uh oh!

costinm commented Feb 27, 2018

Uh oh!

ldemailly commented Feb 27, 2018

Uh oh!

ldemailly commented Feb 28, 2018

Uh oh!

ldemailly commented Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldemailly commented Feb 28, 2018

Uh oh!

geeknoid commented Feb 28, 2018

Uh oh!

istio-testing commented Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geeknoid commented Feb 28, 2018

Uh oh!

geeknoid commented Feb 28, 2018

Uh oh!

geeknoid commented Feb 28, 2018

Uh oh!

costinm commented Feb 28, 2018

Uh oh!

ldemailly commented Feb 28, 2018

Uh oh!

istio-merge-robot commented Feb 28, 2018

Uh oh!

ldemailly commented Feb 28, 2018

Uh oh!

istio-merge-robot commented Feb 28, 2018

Uh oh!

istio-merge-robot commented Feb 28, 2018

Uh oh!

douglas-reid Mar 1, 2018

Choose a reason for hiding this comment

Uh oh!

ldemailly commented Mar 1, 2018

Uh oh!

Reviewers

geeknoid commented Feb 24, 2018 •

edited

Loading

ldemailly commented Feb 24, 2018 •

edited

Loading

ldemailly commented Feb 24, 2018 •

edited

Loading

ldemailly commented Feb 28, 2018 •

edited

Loading

istio-testing commented Feb 28, 2018 •

edited

Loading