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

Dataflow SDK to support launch beam python code or template #833

Merged
merged 6 commits into from
Feb 27, 2019

Conversation

hongye-sun
Copy link
Contributor

@hongye-sun hongye-sun commented Feb 17, 2019

This change is Reviewable

@vicaire
Copy link
Contributor

vicaire commented Feb 20, 2019

/lgtm

# limitations under the License.
import re

def is_gcs_path(path):
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this function is in a storage directory, maybe we should add verification.
There are name requirements for GCS path: https://cloud.google.com/storage/docs/naming
For example, the following two requirements are important and easy to check:

  1. Bucket names must start and end with a number or letter.
  2. Bucket names must contain 3 to 63 characters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I purposely skip those checks in a lot of places because we are passing user provided data to GCP APIs and the APIs will validate the it and report bad request error to the user and we will show it in the log. The validation will be a dup work from the server code did and we have to maintain it if any rules are changed in the future. The check I have it here is for a logic in my code which differs from gcp path and local path.

@vicaire
Copy link
Contributor

vicaire commented Feb 22, 2019

This looks good to me once @gaoning777 are addressed.

@k8s-ci-robot k8s-ci-robot removed the lgtm label Feb 26, 2019
@vicaire
Copy link
Contributor

vicaire commented Feb 26, 2019

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vicaire

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@hongye-sun
Copy link
Contributor Author

/test kubeflow-pipeline-e2e-test

@hongye-sun
Copy link
Contributor Author

/retest

2 similar comments
@hongye-sun
Copy link
Contributor Author

/retest

@hongye-sun
Copy link
Contributor Author

/retest

@k8s-ci-robot k8s-ci-robot merged commit ea26a57 into kubeflow:master Feb 27, 2019
cheyang pushed a commit to alibaba/pipelines that referenced this pull request Mar 28, 2019
…#833)

* initial files for dataflow commands

* dataflow commands

* add launch python command

* Support launch_python command

* Use display API and default outputs to /tmp/output folder
Linchin pushed a commit to Linchin/pipelines that referenced this pull request Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants