Zipkin: Allow SetTag even when the CS annotation is missing (#1414) #1418
Conversation
clguiman
commented
Aug 9, 2017
clguiman
commented
- Removed the check for CS annotation in ZipkinSpan::SetTag
- Removed ZipkinSpan::hasCSAnnotation (SetTag was the only place where it was used)
- Updated Zipkin Tracer unit tests
|
@rshriram @fabolive please review. cc @adriancole if you have time for review. |
|
@mattklein123 I will review this later today. |
|
LGTM, will defer final review to @fabolive |
clguiman
force-pushed
the
zipkin-set-tag-always
branch
from
c2d49b0 to
312839e
Compare
|
I had a compilation error in zipkin_tracer_impl_test (I missed it due to other tests failing locally), so I amended the commit |
|
@clguimanMSFT thanks in the future once you open PRs please don't rebase (just push new commits). It's easier to review. Thank you. |
ci/bazel/get_workspace_status
Outdated
| @@ -0,0 +1 @@ | |||
| /source/bazel/get_workspace_status | |||
There was a problem hiding this comment.
Can you revert this change and below?
There was a problem hiding this comment.
sorry for that, I have reverted the change
|
Is there some easy way I can just run specific tests? Currently I have a lot of test failures locally and I would like to run just the tests directly affected by my change. |
|
If you are using the CI container directly, IIRC we don't have an option to override the tests. You could hack the script that launches the tests, for example: https://github.com/lyft/envoy/blob/master/ci/do_ci.sh#L49 |
|
@clguimanMSFT this is passing tests now. Are you OK to commit or did you want to look at something else? |
|
@mattklein123 I'm OK with the commit. |
Refers to github issue envoyproxy#1418
