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

SDK - Containers - Do not create GCS bucket unless building the image #1938

Conversation

Ark-kun
Copy link
Contributor

@Ark-kun Ark-kun commented Aug 23, 2019

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 Reviewable

@Ark-kun Ark-kun requested a review from gaoning777 August 23, 2019 02:55
@Ark-kun Ark-kun force-pushed the SDK---Containers---Only-create-bucker-during-image-building branch from 4b7e44d to b4e58f7 Compare August 23, 2019 21:14
@Ark-kun
Copy link
Contributor Author

Ark-kun commented Sep 16, 2019

/retest

@Ark-kun Ark-kun force-pushed the SDK---Containers---Only-create-bucker-during-image-building branch 2 times, most recently from daed22d to 38085f8 Compare September 17, 2019 03:16
@Ark-kun Ark-kun requested a review from numerology September 23, 2019 20:01
@Ark-kun
Copy link
Contributor Author

Ark-kun commented Sep 24, 2019

@numerology I've talked to @gaoning777 and he agreed with the idea of this refactoring.

@numerology
Copy link

/lgtm

Left one nit comment.

@gaoning777
Copy link
Contributor

@numerology I've talked to @gaoning777 and he agreed with the idea of this refactoring.

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.
However, this refactoring does change the constructor signature(default_image_name). I understand that this class is not exposed in the init.py but there might still be users using it. I'm suggesting to not break that change.

@k8s-ci-robot k8s-ci-robot removed the lgtm label Sep 24, 2019
@Ark-kun
Copy link
Contributor Author

Ark-kun commented Sep 24, 2019

However, this refactoring does change the constructor signature(default_image_name). I understand that this class is not exposed in the init.py but there might still be users using it. I'm suggesting to not break that change.

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.
@Ark-kun Ark-kun force-pushed the SDK---Containers---Only-create-bucker-during-image-building branch from 0aface8 to aa1fa27 Compare September 24, 2019 20:40
@Ark-kun
Copy link
Contributor Author

Ark-kun commented Sep 24, 2019

However, this refactoring does change the constructor signature(default_image_name).

Changed that back. @gaoning777

@gaoning777
Copy link
Contributor

/lgtm

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Sep 24, 2019

/approve
/retest

@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Sep 24, 2019

/test kubeflow-pipeline-sample-test

1 similar comment
@Ark-kun
Copy link
Contributor Author

Ark-kun commented Sep 25, 2019

/test kubeflow-pipeline-sample-test

@gaoning777
Copy link
Contributor

Do you want to merge with master? Looks like the samples tests are failing consistently.

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Sep 25, 2019

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.

@k8s-ci-robot k8s-ci-robot merged commit 51585a7 into kubeflow:master Sep 25, 2019
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