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

Skip unnecessary steps in USE_BAZEL builds on TravisCI #622

Merged
merged 2 commits into from
Apr 27, 2018

Conversation

yugui
Copy link
Member

@yugui yugui commented Apr 26, 2018

No description provided.

.travis.yml Outdated
@@ -12,34 +12,55 @@ cache:
- $HOME/.cache/_grpc_gateway_bazel
before_install:
- if [ "${USE_BAZEL}" = true ]; then ./.travis/install-bazel.sh 0.12.0; fi
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you make this line match the test style you used below?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean just if test "${USE_BAZEL} = true; then? Or are you thinking about something like test "${USE_BAZEL}" = true && ./.travis/install-bazel.sh?

I did not touch those steps for two reasons.

  • Bazel-related steps are less than others. So they does not bloat up the configuration very much even if I left them as they are.
  • I can't simply replace them with test "${USE_BAZEL}" = true && ./.travis/install-bazel.sh ... because whole of the step must have exit code 0 on -z ${USE_BAZEL}.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The 2nd one is is the reason I was looking for. WDYT about consolidating version numbers into the env section?

Copy link
Member Author

@yugui yugui Apr 27, 2018

Choose a reason for hiding this comment

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

OK. Then, are you ok for keeping if [ "${USE_BAZEL} = true ]; then as is?

WDYT about consolidating version numbers into the env section?

I think it is a good opportunity to do it. I'll push another commit later.

@codecov-io
Copy link

Codecov Report

Merging #622 into master will increase coverage by 0.98%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #622      +/-   ##
==========================================
+ Coverage   59.28%   60.26%   +0.98%     
==========================================
  Files          28       30       +2     
  Lines        2780     3030     +250     
==========================================
+ Hits         1648     1826     +178     
- Misses        971     1039      +68     
- Partials      161      165       +4
Impacted Files Coverage Δ
...oc-gen-grpc-gateway/descriptor/grpc_api_service.go 0% <0%> (ø)
...-grpc-gateway/descriptor/grpc_api_configuration.go 29.62% <0%> (ø)
protoc-gen-grpc-gateway/descriptor/registry.go 76.11% <0%> (+1.4%) ⬆️
protoc-gen-grpc-gateway/gengateway/generator.go 43.9% <0%> (+2.11%) ⬆️
protoc-gen-grpc-gateway/descriptor/types.go 52.41% <0%> (+3.63%) ⬆️
protoc-gen-swagger/main.go 33% <0%> (+5.6%) ⬆️
protoc-gen-grpc-gateway/descriptor/services.go 82.55% <0%> (+8.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b502d2d...df475f5. Read the comment docs.

Copy link
Collaborator

@achew22 achew22 left a comment

Choose a reason for hiding this comment

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

LGTM feel free to merge

@yugui yugui merged commit f252496 into master Apr 27, 2018
@yugui yugui deleted the feature/faster-bazel branch April 27, 2018 15:29
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.

4 participants