Extract building dist package out of prestoadmin_installer #195
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Build binary files seems to be a better task for the build tool (like make) than for the product tests framework. Moreover product tests framework was caching the result in the `tmp/installer` directory so when the changes were introduced to presto-admin, package was not rebuild. So it was easy to run product tests with the old version of product-tests. Instead of that building distribution package was moved to Makefile. Moreover, disregarding how this file is created it will be stored in `dist` directory. No more need for having `tmp/installer`. Fixes prestodb#189 Pull request: prestodb#195
| @@ -17,11 +17,12 @@ env: | |||
| - PRODUCT_TEST_GROUP=5 | |||
| - PRODUCT_TEST_GROUP=6 | |||
| - PRODUCT_TEST_GROUP=7 | |||
| install: | |||
| before_script: | |||
| - pip install --upgrade pip==6.1.1 | |||
There was a problem hiding this comment.
not related to this change, but the pip version in requirements.txt is 7.1.2 (and maybe should be bumped up to the latest version of pip)
kokosing
force-pushed
the
feature_extract_dist_package
branch
from
1d451dd to
3b95e7b
Compare
|
I applied the comments, but I am not going to push it until we have travis working so I am sure I didn't break anything. |
| docker-dist: docker-dist-online | ||
|
|
||
| docker-dist-online: clean-build clean-pyc | ||
| docker run --rm -v `pwd`:/workdir -w /workdir -u `id -u`:`id -g` teradatalabs/presto-admin-devenv:1 make dist-online |
There was a problem hiding this comment.
Can you please update the readme to cover the following:
- When you'd want to run dist-<online|offline> vs docker-dist<online|offline>
- What python gets used to build the installers when you make any of the above targets
- Where the installer ends up.
Thanks!
|
This is an awesome clean-up! Thanks so much for taking care of this. I am delighted to see all the magic in prestoadmin_installer.py being removed. |
|
Travis is working-ish; can you please rebase this after we release presto-admin so we can get this in? |
|
I am going to rebase this pull request, so far I extracted this: #320 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Extract building dist package out of prestoadmin_installer
Build binary files seems to be a better task for the build tool (like make) than
for the product tests framework. Moreover product tests framework was
caching the result in the
tmp/installerdirectory so when the changeswere introduced to presto-admin, package was not rebuild. So it was easy
to run product tests with the old version of product-tests.
Instead of that building distribution package was moved to Makefile.
Moreover, disregarding how this file is created it will be stored in
distdirectory. No more need for havingtmp/installer.Fixes #189