-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
[BEAM-11608] Development environment set up automation #14584
[BEAM-11608] Development environment set up automation #14584
Conversation
Co-authored-by: Isidro Martinez <[email protected]>
…08-dev-env-bash-script
Hm is this intended to replace #14475? Should we close that one? |
Thank you! Copying my comment from that PR over here:
|
Yes, this is a good idea. I like the idea of having a single place to define requirements. |
Any update on this work Ben? |
Yes, @fernando-wizeline and I are checking to have a separate file with package dependencies and install them in both scripts from that list. We expect to push new commits today. |
Run RAT PreCommit |
retest this please |
Hi @tysonjh! Do you know who can help us to merge this? |
@TheNeuralBit is out of office right now, I think he'd be the best person and will return on Monday. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall. I have a few suggestions and questions.
local-env-setup.sh
Outdated
fi | ||
|
||
# Running on Mac | ||
if [ "$(uname -s)" = "Darwin" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this should be structured as:
kernelname = $(uname -s)
if kernelname == "Linux"
...
elseif kernelname == "Darwin"
...
else
echo "Unrecognized Kernel Name: $kernelname"
Right now if we don't hit either if the script just exits silently.
id: local_env_install_mac | ||
- name: "Gradle check" | ||
run: "./gradlew checkSetup" | ||
id: local_env_install_gradle_check_mac |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have these been tested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Brian! Yeah, this has been tested successfully.
inputs: | ||
runDataflow: | ||
description: 'Type "true" if you want to run Dataflow tests (GCP variables must be configured, check CI.md)' | ||
default: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a copy-paste error, I don't think a runDataflow
input is relevant here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @fernando-wizeline, just wanted to double-check you saw this and the comment below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Brian @TheNeuralBit !
Yeah, I already applied both changes and will upload them today.
pull_request: | ||
branches: ['master', 'release-*'] | ||
tags: 'v*' | ||
paths: ['sdks/python/**', 'model/**'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a copy-paste error, I don't think we should restrict this to run only when the Python SDK changes. I'm not sure if there's a reasonable set of paths we can restrict this to, we may just want to run it on every PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps changes to the following paths should trigger it:
- dev-support/**
- buildSrc/**
- **/build.gradle
- sdks/python/setup.py
- sdks/python/tox.ini
That's almost certainly not exhaustive, but maybe enough.
What is the next step on this PR? |
Hi Ahmet! |
Hi @TheNeuralBit @aaltay ! Thanks a lot for the help! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM aside from one nit. Thanks for figuring this out!
dev-support/docker/pkglist
Outdated
@@ -36,5 +36,4 @@ python3.6 | |||
python3.7 | |||
python3.8 | |||
tox | |||
virtualenv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: since this will affect the dev docker container as well please add virtualenv to the pip install line there:
beam/dev-support/docker/Dockerfile
Line 90 in dd15d59
RUN pip3 install grpcio-tools mypy-protobuf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh good catch!
Dependency added.
Thanks for the heads up!
Codecov Report
@@ Coverage Diff @@
## master #14584 +/- ##
==========================================
+ Coverage 83.62% 83.79% +0.16%
==========================================
Files 445 435 -10
Lines 59027 58423 -604
==========================================
- Hits 49360 48954 -406
+ Misses 9667 9469 -198
Continue to review full report at Codecov.
|
Thank you! |
Added local-env-setup.sh to automate the installation of local environment dependencies for new Beam users.
ValidatesRunner
compliance status (on master branch)Examples testing status on various runners
Post-Commit SDK/Transform Integration Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.