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: remove option to set volume snapshotter #308

Conversation

Callisto13
Copy link
Member

What this PR does / why we need it:

Flintlock does not handle other snapshotters well, so we should be
opinionated about this. There is no real reason to support others.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #180

Special notes for your reviewer:
I have done the lightest possible change here: simply removed the flag and set the default at the point we wire the containerd config. I chose to do this here rather than in the ImageService (there is a little getSnapshotter thing) so that all containerd config was grouped upfront for easier tracing.

Checklist:

  • squashed commits into logical changes
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

@Callisto13 Callisto13 added the kind/feature New feature or request label Dec 6, 2021
@Callisto13 Callisto13 force-pushed the you-can-have-any-snapshotter-so-long-as-it-is-devmapper branch 2 times, most recently from d15012d to 9f138c9 Compare December 6, 2021 14:58
@codecov-commenter
Copy link

codecov-commenter commented Dec 6, 2021

Codecov Report

Merging #308 (8c317e7) into main (79a648b) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #308   +/-   ##
=======================================
  Coverage   57.95%   57.95%           
=======================================
  Files          51       51           
  Lines        2509     2509           
=======================================
  Hits         1454     1454           
  Misses        939      939           
  Partials      116      116           

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 79a648b...8c317e7. Read the comment docs.

richardcase
richardcase previously approved these changes Dec 13, 2021
Copy link
Member

@richardcase richardcase left a comment

Choose a reason for hiding this comment

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

I like that its set outside of the image service....just in case another snapshotter comes along that we can use.

Flintlock does not handle other snapshotters well, so we should be
opinionated about this. There is no real reason to support others.
@Callisto13 Callisto13 force-pushed the you-can-have-any-snapshotter-so-long-as-it-is-devmapper branch from 9f138c9 to 0441294 Compare December 13, 2021 10:02
@Callisto13 Callisto13 merged commit 45622b0 into liquidmetal-dev:main Dec 15, 2021
@Callisto13 Callisto13 deleted the you-can-have-any-snapshotter-so-long-as-it-is-devmapper branch December 15, 2021 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Only allow devmapper for volumes
3 participants