-
Notifications
You must be signed in to change notification settings - Fork 8
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
#2 Add Github Action build workflow #40
Conversation
# This is the 1st commit message: # This is a combination of 4 commits. # This is the 1st commit message: #2 Add Maven build workflow # This is the commit message #2: #2 update build workflow # This is the commit message #3: #2 add branch checkout to build workflow # This is the commit message #4: #2 Add on event to build workflow # This is the commit message #2: #2 Remove unused executions # This is the commit message #3: #2 Focus on build # This is the commit message #4: #2 Remove build execution where not needed # This is the commit message #5: #2 debug failing module # This is the commit message #6: #2 Remove unused target folder copy # This is the commit message #7: #2 Build spark and jenkins docker images # This is the commit message #8: #2 Retry full build # This is the commit message #9: #2 Omitting module # This is the commit message #10: #2 Fix for out of disk space # This is the commit message #11: #2 tagging docker images # This is the commit message #12: #2 Remove Temporarily remove Docker module # This is the commit message #13: #2 Build update # This is the commit message #14: #2 Build update # This is the commit message #15: #2 move chart dry-runs to IT profile # This is the commit message #16: #2 curl delta-hive assembly in docker build # This is the commit message #17: #2 cache m2 repo # This is the commit message #18: #2 prune docker build cache between images to save space # This is the commit message #19: #2 add maven build-cache to GH cache # This is the commit message #20: #2 run clean goal in build to clear docker cache # This is the commit message #21: #2 set maven caches to always save even if the build failed # This is the commit message #22: #2 adjust number of docker modules built # This is the commit message #23: #2 use the same cache for .m2 even if poms change # This is the commit message #24: #2 change from `save-always` flag to `if: always()` see actions/cache#1315 # This is the commit message #25: #2 further reduce docker images being built # This is the commit message #26: #2 disable modules that depend on helm charts # This is the commit message #27: #2 use maven wrapper # This is the commit message #28: #2 restore modules to test build-cache # This is the commit message #29: #2 fix build of modules with intra-project chart dependencies # This is the commit message #30: #2 use explict .m2 repo cache so we can fall-back to older caches # This is the commit message #31: #2 save maven caches on build failure
.github/workflows/build.yml
Outdated
type: string | ||
default: '2-build-action' | ||
push: | ||
branches: [ "2-build-action" ] |
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.
S: Should update these to dev
. It might also make sense to have this run when a PR to dev is open and use the PR branch as the input build branch. But that can come in later iterations once we get the dev branch building locally.
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.
Good catch
- Switch to dev branch
@@ -4,8 +4,9 @@ FROM ${DOCKER_BASELINE_REPO_ID}boozallen/aissemble-spark:${VERSION_AISSEMBLE} | |||
|
|||
LABEL org.opencontainers.image.source = "https://github.com/boozallen/aissemble" | |||
|
|||
RUN curl -L https://github.com/delta-io/connectors/releases/download/v0.6.0/delta-hive-assembly_2.12-0.6.0.jar \ | |||
-o "${SPARK_HOME}"/jars/delta-hive-assembly_2.12-0.6.0.jar |
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.
A: Should probably pass the connector version as a docker build arg from the POM so we can drive version numbers from Maven.
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.
Agree
- Move version to build arg
extensions/extensions-docker/pom.xml
Outdated
<module>aissemble-model-training-api-containers</module> | ||
<!-- <module>aissemble-versioning</module>--> | ||
<!-- <module>aissemble-mlflow</module>--> | ||
<!-- <module>aissemble-model-training-api-containers</module>--> |
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.
S: Revert this as local developers have the appropriate access keys setup to be able to build all of the images.
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.
Agree
- uncomment modules
Don't forget to rebase. I reviewed but didn't approve as I probably helped author too many of the changes to be a valid reviewer. |
@@ -5,9 +5,7 @@ LABEL org.opencontainers.image.source = "https://github.com/boozallen/aissemble" | |||
|
|||
USER root | |||
|
|||
COPY ./target/cacerts/* /usr/local/share/ca-certificates/ | |||
|
|||
RUN update-ca-certificates && apt-get update && apt-get install -y --no-install-recommends apt-utils |
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.
Isn't this gonna make the images unbuildable from a BAH device since they needs the certs?
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.
If you log out of your VPN to do the build you should be able to build just fine. But we can't really install certs specific to one organization. I think long-term it'd be nice to find some sort of just-in-time solution that allows VPN CA certs to be made available without actually modifying the final image.
|
|
||
# git and build-essential are used during pip install of some dependencies (e.g. python-deequ) | ||
# software-properties-common and *-dev dependencies are needed to build an updated version of Python | ||
# as Python 3.11.4 is packaged by default with the base spark-py image | ||
RUN update-ca-certificates && apt-get update -y && apt-get install --assume-yes \ |
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.
S: looks like we might have missed the update-ca-certificates
here
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.
Good catch
- Remove update-ca-certificates
There will be a follow on to this PR to handle certain build issues such as the limited amount of disk space on the Action Runner.