Fix our build #524
Merged
Fix our build #524
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pull Request Test Coverage Report for Build 1838
💛 - Coveralls |
tneymanov
reviewed
Oct 31, 2019
Collaborator
tneymanov
left a comment
tneymanov
left a comment
There was a problem hiding this comment.
Again, I would had preferred updating all the tests, but for now this works. we can fix the build and update the tests to supply region explicitly prior to the release.
setup.py
Outdated
| 'protobuf>=3.6.1', | ||
| 'mmh3<2.6', | ||
| # Need to explicitly install v<=1.14.0. apache-beam requires | ||
| # google-cloud-pubsub 0.39.1, which relies on google-cloud-core<0.30dev, |
tneymanov
approved these changes
Oct 31, 2019
run_presubmit.sh
Outdated
| pip install pylint | ||
| python -m pip install pylint | ||
| fi | ||
| pylint gcp_variant_transforms |
Collaborator
There was a problem hiding this comment.
This may not be needed, but for consistency, maybe modify this to python -m pylint?
samanvp
commented
Nov 1, 2019
run_presubmit.sh
Outdated
| pip install pylint | ||
| python -m pip install pylint | ||
| fi | ||
| pylint gcp_variant_transforms |
setup.py
Outdated
| 'protobuf>=3.6.1', | ||
| 'mmh3<2.6', | ||
| # Need to explicitly install v<=1.14.0. apache-beam requires | ||
| # google-cloud-pubsub 0.39.1, which relies on google-cloud-core<0.30dev, |
allieychen
approved these changes
Nov 1, 2019
Contributor
allieychen
left a comment
allieychen
left a comment
There was a problem hiding this comment.
Thank you Saman, LGTM!
kemp-google
reviewed
Nov 4, 2019
| # A helper script for ensuring all checks pass before submitting any change. | ||
|
|
||
| echo ========== Running unit tests. | ||
| if [[ -z `which coverage` ]];then |
There was a problem hiding this comment.
Just a general question: is it relatively fast to just install the tools required? eg, if this were apt, I'd just install everything rather than check/install.
Not something to change in this PR, just a general question about pip.
kemp-google
approved these changes
Nov 4, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Two issues needed to be fixed:
pip installcommands withpython -m pip installdue to a new cloud-sdk:slim.--regionto all integrations tests due to new Apache Beam requires to send--regionto all pipeline executions.Note that this PR only fixes the build. We need to also enforce users to provide
--regionflag when running Variant Transforms. For more details refer to #525