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

feat: add docker layer caching support on machine executors #147

Merged
merged 7 commits into from Sep 29, 2022

Conversation

a-cordier
Copy link
Contributor

see #146

@ghost
Copy link

ghost commented Sep 22, 2022

👇 Click on the image for a new way to code review
  • Make big changes easier — review code in small groups of related files

  • Know where to start — see the whole change at a glance

  • Take a code tour — explore the change with an interactive tour

  • Make comments and review — all fully sync’ed with github

    Try it now!

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map Legend

@a-cordier a-cordier force-pushed the feat-146 branch 2 times, most recently from 1616a42 to c1727e0 Compare September 27, 2022 05:34
@KyleTryon
Copy link
Contributor

Hey @a-cordier Thanks for the contribution!
I see here the Docker Layer Caching boolean was added to the machine executor type.

Docker Layer Caching is actually available on the Docker Container as well: https://circleci.com/docs/docker-layer-caching

We probably could do some very advanced typescript stuff to make it so that you require the setup_remote_docker step on the docker-based job implementing DLC, but i think for now we could potentially get away with adding the boolean to the parent Executor class and extend it down to both.

CC @Jaryt for opinion as well.

@a-cordier
Copy link
Contributor Author

@KyleTryon Thanks for the prompt review ! Does the docker_layer_caching property applies to (MacOS|Windows) executors ? I can see that they both extend the Executor class.

@a-cordier
Copy link
Contributor Author

Just figured out that dlc is implemented with a command for docker executors.

I've updated the PR to add dlc support on the command as well

Copy link
Contributor

@KyleTryon KyleTryon left a comment

Choose a reason for hiding this comment

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

Well done 👏 . And thank you for looking into the config reference to resolve this. I had to double check that myself. I am starting the tests now.

edit: oops, just noticed the branch is out of date. Going to update that first, fingers crossed for no conflicts

Copy link
Contributor

@KyleTryon KyleTryon left a comment

Choose a reason for hiding this comment

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

@KyleTryon KyleTryon self-requested a review September 29, 2022 14:20
@codecov
Copy link

codecov bot commented Sep 29, 2022

Codecov Report

Base: 97.26% // Head: 97.28% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (d9bc746) compared to base (d9db669).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #147      +/-   ##
==========================================
+ Coverage   97.26%   97.28%   +0.02%     
==========================================
  Files          61       61              
  Lines         658      663       +5     
  Branches       64       65       +1     
==========================================
+ Hits          640      645       +5     
  Misses          7        7              
  Partials       11       11              
Impacted Files Coverage Δ
...nents/Commands/exports/Native/SetupRemoteDocker.ts 100.00% <100.00%> (ø)
...ib/Components/Executors/exports/MachineExecutor.ts 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@KyleTryon KyleTryon merged commit 34a9dfa into CircleCI-Public:main Sep 29, 2022
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.

None yet

2 participants