-
Notifications
You must be signed in to change notification settings - Fork 80
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
Refactored the OSB build system #632
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cp -a opensearch-benchmark-git/* ./ | ||
echo "Disable VERSION arg to enter docker build test mode" | ||
PLATFORM=${{ matrix.platform }} | ||
PLATFORM=`echo $PLATFORM | tr '/' '-'` | ||
docker buildx build --platform ${{ matrix.platform }} --build-arg BUILD_ENV=testing --build-arg BUILD_DATE=`date -u +%Y-%m-%dT%H:%M:%SZ` -f "docker/Dockerfile" -t "osb/osb-$PLATFORM" -o type=docker . | ||
docker images | grep "osb/osb-$PLATFORM" | ||
tag=osb/osb-`echo ${{ matrix.platform }} | tr '/' '-'` | ||
set -x |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand your changes here but this change combined with the Dockerfile change basically cause the docker-build check to be inaccurate.
It will only check docker build based on merged code, instead of the commit sent with the PR. You can only detect issues once you merge the code, which defeat the purpose of PR check.
My original design was meant to have test mode testing the PR commit, while prod mode use the original pypi method. I think we should bring back the test mode, but only change prod mode to build from source instead of pulling from pypi, which should be the best of both approaches. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The updated changes should pull in the PR commit now.
19b8175
to
6ebf085
Compare
e82d2c6
to
bb0e874
Compare
Seems like docker-lint does not pass. |
05757c5
to
7ad5d76
Compare
Signed-off-by: Govind Kamat <govkamat@amazon.com>
ARG VERSION | ||
ARG BUILD_ENV=production | ||
# | ||
# Stage 1: build packages and compile where needed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did we get any significant size reduction in final image by splitting into multi-stage? It is fairly simple dockerfile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See this comment:
#632 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Size reduction is as noted in the link above. However, the objective of this PR is not so much the size reduction as it is to ensure the full release build process is run when changes are checked in and errors are caught at that time rather than during release. Also, the standalone development build has been revamped and simplified as well. Finally, the integ test pathway has been separated out too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @gkamat !
Signed-off-by: Govind Kamat <govkamat@amazon.com>
Description
Restructured the entire OSB build system, including standalone, Docker and via GitHub actions.
Testing
Tested out most combinations of the workflows above.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.