Skip to content
This repository was archived by the owner on Mar 26, 2026. It is now read-only.

Migrate minio-py to use latest release 2.2.4#80

Merged
nitisht merged 1 commit into
minio:masterfrom
harshavardhana:python
Jul 6, 2017
Merged

Migrate minio-py to use latest release 2.2.4#80
nitisht merged 1 commit into
minio:masterfrom
harshavardhana:python

Conversation

@harshavardhana

@harshavardhana harshavardhana commented Jul 6, 2017

Copy link
Copy Markdown
Member

No description provided.

@nitisht nitisht requested review from krisis and nitisht July 6, 2017 00:59
@nitisht nitisht mentioned this pull request Jul 6, 2017
Closed
@harshavardhana harshavardhana force-pushed the python branch 3 times, most recently from 90b3f1f to 37d6a2e Compare July 6, 2017 06:32
nitisht
nitisht reviewed Jul 6, 2017
View reviewed changes
Comment thread build/py/minio-py.sh
}

installFunctionalTest() {
curl https://raw.githubusercontent.com/minio/minio-py/${MINIO_PY_SDK_VERSION}/tests/functional/tests.py > /usr/bin/minio-py-functional

@nitisht nitisht Jul 6, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of curl is it a good idea to pull from a specific GitHub tag (release) so we don't get to the issue of having inconsistent tests and SDKs version.

@harshavardhana harshavardhana Jul 6, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That is what its doing its picking from the tag. @nitisht

@nitisht nitisht Jul 6, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I missed that. Thanks @harshavardhana

nitisht
nitisht reviewed Jul 6, 2017
View reviewed changes

@nitisht nitisht left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM overall, just one comment.

@harshavardhana

harshavardhana commented Jul 6, 2017

Copy link
Copy Markdown
Member Author 
Upload a small file through putObject
Upload a largefile through multipart upload
Perform a server side copy of an object
Perform a server side copy of an object with pre-conditions and fail
Upload a streaming object of 1MiB
Upload a streaming object of 11MiB
Stat on the uploaded object check if it exists
Download a full object, iterate on response to save to disk
Download a full object and save locally at path
Listing using ListObjects
Listing using ListObjectsV2
Prepare for remove_objects() test.
Performing remove_objects() test.
Removing objects worked as expected.
Deleting buckets and finishing tests.

docker run --net="host" -e SERVER_ENDPOINT=10.0.0.13:9000 -e ACCESS_KEY=USWUXHGYZQYFYFFIT3RE -e SECRET_KEY=MOJRH0mkL1IPauahWITSVvyDrQbEEIwljvmxdq03 -e ENABLE_HTTPS=0 play.minio.io/mint:travis-37d6a2eb0a2b6d859d14a87c05738dbec4628fe9

@harshavardhana harshavardhana force-pushed the python branch 2 times, most recently from 3933a1e to 49de03e Compare July 6, 2017 06:51
@nitisht nitisht requested a review from vadmeste July 6, 2017 17:47
vadmeste
vadmeste reviewed Jul 6, 2017
View reviewed changes
Comment thread build/py/install.sh
install() {
apt-get install -yq python3
apt-get install -yq python3-pip
update-alternatives --install /usr/bin/python python /usr/bin/python3.5 1

@vadmeste vadmeste Jul 6, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you add a comment why this is needed ? not able to guess (is this because there is python 2 by default in ubuntu installation ?)

@harshavardhana harshavardhana Jul 6, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No because there is no /usr/bin/python and its needed for pretty much all python applications. Also the functional tests which we download /usr/bin/env python would fail without this.

I don't know if this needs a comment.

@vadmeste vadmeste Jul 6, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

oh you mean python3 won't create /usr/bin/python symlink, I think you need to add a comment for this, this is not clear from the first glance.

@harshavardhana harshavardhana Jul 6, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yep it doesn't create.

vadmeste
vadmeste approved these changes Jul 6, 2017
View reviewed changes

@vadmeste vadmeste left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM (one comment needed otherwise)

@nitisht

nitisht commented Jul 6, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Are all the comments addressed?

@harshavardhana

harshavardhana commented Jul 6, 2017

Copy link
Copy Markdown
Member Author

Are all the comments address?

One comment adding what @vadmeste asked.. one minute.

@harshavardhana
Migrate minio-py to use latest release 2.2.4
3ac337a 
Also fix unicode support for PYTHON by adding LANG env.

docker-library/python@dbe3e24
@nitisht nitisht merged commit b9bbd61 into minio:master Jul 6, 2017
@harshavardhana harshavardhana deleted the python branch July 6, 2017 23:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@vadmeste vadmeste vadmeste approved these changes

@krisis krisis krisis approved these changes

@nitisht nitisht nitisht approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@harshavardhana @nitisht @vadmeste @krisis