Skip to content

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

Merged
domire8 merged 6 commits into
developfrom
ci/test
Apr 2, 2024
Merged

ci: use caching from docker to run test in CI#169
domire8 merged 6 commits into
developfrom
ci/test

Conversation

@domire8

@domire8 domire8 commented Mar 26, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

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

domire8 commented Mar 26, 2024

Copy link
Copy Markdown
Member Author

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
domire8 commented Mar 26, 2024
View reviewed changes
Comment thread source/state_representation/test/tests/test_analog_io.cpp Outdated
@domire8

domire8 commented Mar 26, 2024

Copy link
Copy Markdown
Member Author

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
View reviewed changes
Comment thread Dockerfile Outdated
COPY --from=apt-dependencies /tmp/apt /
COPY --from=dependencies /tmp/deps /usr
COPY . /src
COPY licenses licenses

@LouisBrunner LouisBrunner Mar 26, 2024

Copy link
Copy Markdown

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

@domire8 domire8 Mar 26, 2024

Copy link
Copy Markdown
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 domire8 force-pushed the ci/test branch 11 times, most recently from 6ae1031 to 97f289c Compare March 27, 2024 07:13
@domire8

domire8 commented Mar 27, 2024

Copy link
Copy Markdown
Member Author

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 mentioned this pull request Mar 27, 2024
Merged
1 task
@domire8 domire8 linked an issue Mar 27, 2024 that may be closed by this pull request
Revamp build structure #159
Closed
eeberhard
eeberhard previously approved these changes Mar 28, 2024
View reviewed changes

@eeberhard eeberhard left a comment

Copy link
Copy Markdown
Member

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

domire8 commented Mar 28, 2024

Copy link
Copy Markdown
Member Author

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
@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.

Reviewers

@LouisBrunner LouisBrunner LouisBrunner left review comments

@eeberhard eeberhard eeberhard left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Revamp build structure

3 participants

@domire8 @LouisBrunner @eeberhard