Skip to content

Newtype OverlayMedium #11735

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

Closed
wants to merge 1 commit into from
Closed

Conversation

stepancheg
Copy link
Contributor

@stepancheg stepancheg commented May 15, 2025

For #11723 OverlayMedium may need a size field. Encoding it in the string will make code too complex, so this PR proposes converting OverlayMedium to a struct.

Even if size should not be added to OverlayMedium, I believe this extra type safety helps, so I'm submitting this PR.

I do not usually write code in Go, so code style feedback is also welcome.

@stepancheg stepancheg force-pushed the overlay-medium branch 3 times, most recently from a5e5c4a to 66a2cc7 Compare May 15, 2025 23:15
@stepancheg stepancheg marked this pull request as ready for review May 15, 2025 23:19
@ayushr2
Copy link
Collaborator

ayushr2 commented May 15, 2025

Consider adding the optional size to Overlay2 type, instead of complicating the OverlayMedium:

// Overlay2 holds the configuration for setting up overlay filesystems for the
// container.
type Overlay2 struct {
rootMount bool
subMounts bool
medium OverlayMedium
}

The new overlay2 format you are proposing is --overlay2=<which mounts>:<medium>,<options...>. The options will be applied irrespective of the medium (self, memory or anonDir). So we should not pack it as part of the medium.

I think this PR would increase code complexity.

@stepancheg
Copy link
Contributor Author

stepancheg commented May 15, 2025

OK, closing this PR, let's continue discussion in #11723.

@stepancheg stepancheg closed this May 15, 2025
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.

2 participants