-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
SDK - Containers - Do not create GCS bucket unless building the image #1938
SDK - Containers - Do not create GCS bucket unless building the image #1938
Conversation
4b7e44d
to
b4e58f7
Compare
/retest |
daed22d
to
38085f8
Compare
@numerology I've talked to @gaoning777 and he agreed with the idea of this refactoring. |
/lgtm Left one nit comment. |
Just for the record, I'm not strongly opinionated about whether fetching the project id/creating bucket in the constructor or in the build function. |
Sure. I'll split that change out. |
Also missing default image name is no longer an error as long as the image name is provided to ContainerBuilder.build.
0aface8
to
aa1fa27
Compare
Changed that back. @gaoning777 |
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Ark-kun 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 |
/test kubeflow-pipeline-sample-test |
1 similar comment
/test kubeflow-pipeline-sample-test |
Do you want to merge with master? Looks like the samples tests are failing consistently. |
I've checked and these were temporary issues with PyPI network. |
Previously, when the user tried to construct
ContainerBuilder
it immediately created a storage bucket even is no image was build. This prevented the creation of default builder instance.Also missing default image name is no longer an error as long as the image name is provided to ContainerBuilder.build.
This change is