osbuild: simplify code by removing osbuild.Mounts type
#373
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
achilleas-k
approved these changes
Jan 16, 2024
There was a problem hiding this comment.
I agree with this.
There was an idea that the abstraction might be expanded to include some useful methods that work on the Mounts type/alias. Another idea was that it might become an interface where different stages can add their own implementations with restrictions.
None of that happened so let's clean it up. It's a low impact change and it removes ambiguities so I'm all for it.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
The rebase on the merge queue seems to be causing issues. Doing a UI rebase to see what's up. |
achilleas-k
force-pushed
the
simplify-osbuild-mounts
branch
from
bb8f601
to
f1dbd21
Compare
|
Ah, of course. The bootupd stage was merged and this PR merges cleanly but not correctly. |
The `osbuild.Mounts` type is just an alias to `[]osbuild.Mount` and it feels like it does not add clarity. From the type it's not clear if it's a slice or a map (like `osbuild.Devices`). While it is nice to abstract things away one needs to know it anyway when constructing the type so this commit removes the type and just exposes/uses `[]osbuild.Mount` directly. Another benefit of this is that the pointers used are now clearer (IMHO) - i.e. `[]Mount` is a slice so no need to indirect it further via `*Mounts` in the function signatures.
mvo5
force-pushed
the
simplify-osbuild-mounts
branch
from
f1dbd21
to
7ee73c0
Compare
achilleas-k
approved these changes
Jan 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A quick drive-by commit for your consideration. I noticed while writing a test for the bootupd stage that the osbuild.Mounts type is not really helping me and hence wonder if we can do away with it. I did a grep and it seems the type is also not used outside images. Feel free to close this is you disagree or are attached to the type but to me the code feels easier to work with without it (and if you agree with this change I would like to see if
osbuild.Devicemight also benefit from this).The
osbuild.Mountstype is just an alias to[]osbuild.Mountand it feels like it does not add clarity. From the type it's not clear if it's a slice or a map (likeosbuild.Devices). While it is nice to abstract things away one needs to know it anyway when constructing the type so this commit removes the type and just exposes/uses[]osbuild.Mountdirectly.Another benefit of this is that the pointers used are now clearer (IMHO) - i.e.
[]Mountis a slice so no need to indirect it further via*Mountsin the function signatures.