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: use caching from docker to run test in CI #169

Merged
merged 6 commits into from
Apr 2, 2024
Merged

ci: use caching from docker to run test in CI #169

merged 6 commits into from
Apr 2, 2024

Conversation

domire8
Copy link
Member

@domire8 domire8 commented Mar 26, 2024

Description

Let's start small, first would be to run the tests with the caching action from docker and github.

Review guidelines

Estimated Time of Review: 5 minutes

Checklist before merging:

  • Confirm that the relevant changelog(s) are up-to-date in case of any user-facing changes

@domire8 domire8 changed the base branch from main to develop March 26, 2024 05:52
@domire8 domire8 force-pushed the ci/test branch 2 times, most recently from 72cbde9 to 172be2d Compare March 26, 2024 09:19
@domire8 domire8 changed the title Ci/test ci: use caching from docker to run test in CI Mar 26, 2024
@domire8 domire8 requested a review from LouisBrunner March 26, 2024 09:20
@domire8
Copy link
Member Author

domire8 commented Mar 26, 2024

As we can see here this works rather okay out of the box, but I have tried it several times now and somehow it always executes the build stage even if the source code didn't change. I'm not sure what is invalidating the cache for that stage

@domire8 domire8 force-pushed the ci/test branch 2 times, most recently from e118d1e to 2554f0e Compare March 26, 2024 16:01
@domire8
Copy link
Member Author

domire8 commented Mar 26, 2024

This seems to work now, thanks @LouisBrunner

@domire8 domire8 requested a review from eeberhard March 26, 2024 16:38
LouisBrunner
LouisBrunner previously approved these changes Mar 26, 2024
Dockerfile Outdated

FROM base as code
WORKDIR /src
COPY --from=apt-dependencies /tmp/apt /
COPY --from=dependencies /tmp/deps /usr
COPY . /src
COPY licenses licenses

Choose a reason for hiding this comment

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

.dockerignore is better for ignoring specific folder

Copy link
Member Author

Choose a reason for hiding this comment

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

When I put stuff into dockerignore, I can't copy them anymore. I just want to be selective with copying to not invalidate caches unintentionally. If you copy . then docker will continue at this line, even though you maybe only changed something in python, so the cpp build is not necessary

@domire8
Copy link
Member Author

domire8 commented Mar 27, 2024

Okay I'm now finally kind of happy with this PR. Instead of having 3 different test actions (source, proto, python) where you'ds see immediately which tests have failed, everything is now tested in the same step and will just fail somewhere during the build action. However, this allows for best caching. To see that, I invite you to check the last 3 successful workflow runs:

  • the first one needed to build the image from scratch and took 13mins
  • the second one had a change in the cpp tests, so just needed to rebuild the cpp sources and tests and took 6mins
  • the third one only needed to rerun the python tests, so took less than a minute.

Leaving those two last commits here for you to see the workflows, then I'll have to remove it again

@domire8 domire8 linked an issue Mar 27, 2024 that may be closed by this pull request
eeberhard
eeberhard previously approved these changes Mar 28, 2024
Copy link
Member

@eeberhard eeberhard left a comment

Choose a reason for hiding this comment

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

Thank you for the additional example commits to see the caching. The actions and workflow are definitely much simpler now.

N.B. after this we will have to update the branch protection rules which currently expect and require the existing actions to run

@domire8
Copy link
Member Author

domire8 commented Mar 28, 2024

N.B. after this we will have to update the branch protection rules which currently expect and require the existing actions to run

I'm not sure, I can't see them under the branch settings. I had the same thoughts as you though

@domire8 domire8 merged commit 5747b2d into develop Apr 2, 2024
4 checks passed
@domire8 domire8 deleted the ci/test branch April 2, 2024 06:03
@github-actions github-actions bot locked and limited conversation to collaborators Apr 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Revamp build structure
3 participants