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

ci: add build step #56

Merged
merged 2 commits into from
Dec 17, 2020
Merged

ci: add build step #56

merged 2 commits into from
Dec 17, 2020

Conversation

xopham
Copy link
Collaborator

@xopham xopham commented Nov 6, 2020

Originally, the Connaisseur docker image is built in multiple later steps of the pipeline. This causes code duplication, maintenance increase, unintuitive failing and unnecessary ci run time.

Fixes #54

@codecov-io
Copy link

codecov-io commented Nov 6, 2020

Codecov Report

Merging #56 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #56   +/-   ##
=======================================
  Coverage   93.15%   93.15%           
=======================================
  Files          14       14           
  Lines         628      628           
=======================================
  Hits          585      585           
  Misses         43       43           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e51ef71...b047c09. Read the comment docs.

@xopham xopham force-pushed the ci/add-build-step branch 3 times, most recently from 18b4c3b to 5828eac Compare November 10, 2020 14:42
Starkteetje
Starkteetje previously approved these changes Nov 11, 2020
with:
name: images
path: images

Copy link
Member

Choose a reason for hiding this comment

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

Would add retention-days: 1

# ignore Markdown files
*.md
# ignore tests
connaisseur/tests/
Copy link
Member

Choose a reason for hiding this comment

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

If you want marginally faster builds, you can also add img/ since that accounts for ~2/3 of the repo size

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

think that is excluded anyways. Checking...

Copy link
Member

Choose a reason for hiding this comment

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

For build size, it is, since its not used. But for speed during building, the whole context (the . for docker build . ) is copied to the docker daemon. Is local copy of few MB, so 🤷‍♂️ , but if we put a 1GB file in that folder you'll notice

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will however only increase the build speed, but good point!

run: |
mkdir images
docker save ${RELEASE_IMAGE}:${RELEASE_VERSION} -o images/${GITHUB_SHA}_image.tar
docker save ${RELEASE_IMAGE}:helm-hook -o images/${GITHUB_SHA}_hook.tar
Copy link
Member

Choose a reason for hiding this comment

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

Maybe out of scope for this PR, but we are not versioning the helm-hook here, so we technically can't change the hook script without changing naming here (or we break older versions on redeployment), right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added to release discussion

Originally, the Connaisseur docker image is built in multiple later steps of the pipeline. This causes code duplication, maintenance increase, unintuitive failing and unnecessary ci run time.

Fixes #54
Separation of python dependencies for development and production. Adding
.dockerignore to exclude unnecessary files. Implementing multi-stage
build to reduce build dependencies.
@phbelitz phbelitz merged commit a7cc5de into develop Dec 17, 2020
@phbelitz phbelitz deleted the ci/add-build-step branch December 17, 2020 13:03
@phbelitz phbelitz mentioned this pull request Dec 18, 2020
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.

Add build step in CI
4 participants