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

Convert release.sh from bash to python [part 1] #6674

Merged
merged 18 commits into from Oct 24, 2018

Conversation

wisechengyi
Copy link
Contributor

@wisechengyi wisechengyi commented Oct 23, 2018

Convert list_prebuilt_wheels and fetch_prebuilt_wheels to python.

Same logic + parallel downloading in python.

For #5551

@wisechengyi wisechengyi changed the title [wip] slowly convert release.sh from bash to python [wip] Incrementally convert release.sh from bash to python Oct 23, 2018
@wisechengyi wisechengyi changed the title [wip] Incrementally convert release.sh from bash to python [wip] Incrementally convert release.sh from bash to python [part 1] Oct 23, 2018
@wisechengyi wisechengyi changed the title [wip] Incrementally convert release.sh from bash to python [part 1] Incrementally convert release.sh from bash to python [part 1] Oct 23, 2018
@wisechengyi wisechengyi changed the title Incrementally convert release.sh from bash to python [part 1] Convert release.sh from bash to python [part 1] Oct 23, 2018
@wisechengyi
Copy link
Contributor Author

This is ready for review. Thanks.

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

While I don't know the whole context, this looks like a great project! Python scripts are much easier to maintain and extend than Bash.

src/python/pants/releases/release.py Show resolved Hide resolved
@wisechengyi wisechengyi requested review from jsirois and removed request for stuhood October 23, 2018 18:07
Copy link
Contributor

@baroquebobcat baroquebobcat left a comment

Choose a reason for hiding this comment

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

Thanks for this.

Wow, fire looks really cool!

src/python/pants/releases/release.py Outdated Show resolved Hide resolved
src/python/pants/releases/release.py Outdated Show resolved Hide resolved
src/python/pants/releases/release.py Show resolved Hide resolved
@jsirois
Copy link
Member

jsirois commented Oct 23, 2018

I'm all in favor of converting release to a language we can reasonably test, so I won't block. That said - this is all a bit simpler perhaps on the prod side using boto3, but definitely on the testing side using moto. Consider tests in a follow-up, and when doing so, consider switching to boto since it does support parallel transfers and convenient testing via moto.

@wisechengyi
Copy link
Contributor Author

Thanks for the recs, John!

@wisechengyi
Copy link
Contributor Author

Going to land this by EOD if there is no further comments.

Copy link
Sponsor Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Thanks Yi!

src/python/pants/releases/release.py Outdated Show resolved Hide resolved
src/python/pants/releases/release.py Show resolved Hide resolved
@wisechengyi wisechengyi merged commit 01c807e into pantsbuild:master Oct 24, 2018
@wisechengyi wisechengyi deleted the 5551 branch October 24, 2018 05:21
wisechengyi added a commit to wisechengyi/pants that referenced this pull request Oct 29, 2018
wisechengyi added a commit that referenced this pull request Oct 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants