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

Added integration test for Stackdriver Storage with autoconfiguration #143

Merged
merged 8 commits into from
Sep 10, 2019

Conversation

saturnism
Copy link
Collaborator

@saturnism saturnism commented Sep 3, 2019

For #130

Added integration test for Zipkin Stackdriver Reporter, which is a different path than the Stackdriver Sender.

If the decryption fails, the target file will have a length of 0.
We need to add assumption that the file is actually valid/parseable by the credentials library.
@saturnism
Copy link
Collaborator Author

Good thing I'm doing this w/ a PR outside of zipkin-gcp repo ;) caught quite a few issues w/ travis and PR. Fixed those in this commit too.

@@ -49,9 +49,10 @@
<zipkin.version>2.16.2</zipkin.version>
<zipkin-reporter.version>2.10.2</zipkin-reporter.version>
<brave.version>5.6.10</brave.version>
<grpc.version>1.22.1</grpc.version>
<grpc.version>1.22.2</grpc.version>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is updated to match armeria's grpc version.

Copy link
Member

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

works for me. thanks Ray!

Copy link
Contributor

@elefeint elefeint left a comment

Choose a reason for hiding this comment

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

LGTM; a couple of optional comments.

@@ -18,7 +18,8 @@ before_install:
# Encrypted service account file for integration testing
# This file is referenced via GOOGLE_APPLICATION_CREDENTIALS env var
# Ex. travis encrypt-file travis/zipkin-gcp-ci-0d7917f58da7.json
- openssl aes-256-cbc -K $encrypted_0d0e64b78c84_key -iv $encrypted_0d0e64b78c84_iv -in travis/zipkin-gcp-ci-0d7917f58da7.json.enc -out travis/zipkin-gcp-ci-0d7917f58da7.json -d
Copy link
Contributor

Choose a reason for hiding this comment

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

In spring-cloud-gcp, we explicitly gate on $TRAVIS_SECURE_ENV_VARS. In principle, TRAVIS_PULL_REQUEST could also be used to conditionally decrypt as it's false when not running on a pull request.

Environment variable reference: https://docs.travis-ci.com/user/environment-variables/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

great idea; i'll do the same.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

there is a similar gate for decrypting file: https://github.com/spring-cloud/spring-cloud-gcp/blob/0513b8784f8828918aacacf18db80a992ae4134c/.travis.yml#L37

i've updated here to use the same gate logic.

@@ -98,7 +98,7 @@
<dependency>
<groupId>org.awaitility</groupId>
<artifactId>awaitility</artifactId>
<version>3.1.6</version>
<version>${awaitability.version}</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

s/awaitability/awaitility

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good catch!

@saturnism
Copy link
Collaborator Author

/cc @adriancole this should be ready to go :) and if #144 passes tests after this merge, then that's ready to go too.

@saturnism saturnism merged commit c164b9c into openzipkin:master Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants