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

FR-2622 customize /etc/fstab #75

Merged
merged 4 commits into from
Sep 2, 2022
Merged

Conversation

jawn-smith
Copy link
Contributor

This PR adds a new fstab section to the customization section of the image definition, and populates the contents of /etc/fstab in the chroot based on the image definition.

@codecov
Copy link

codecov bot commented Aug 22, 2022

Codecov Report

Merging #75 (c3832c3) into feature/classic-image-redesign (a38a5dc) will not change coverage.
The diff coverage is 100.00%.

@@                       Coverage Diff                        @@
##           feature/classic-image-redesign       #75   +/-   ##
================================================================
  Coverage                          100.00%   100.00%           
================================================================
  Files                                   9         9           
  Lines                                1526      1555   +29     
================================================================
+ Hits                                 1526      1555   +29     
Impacted Files Coverage Δ
internal/statemachine/image_definition.go 100.00% <ø> (ø)
internal/statemachine/classic_states.go 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.

Copy link
Collaborator

@sil2100 sil2100 left a comment

Choose a reason for hiding this comment

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

This is good! Small inline formatting suggestion, but besides that it's all good. I'm thinking a bit about the image-definition format a bit, but I think this all 'fits' so far.

internal/statemachine/image_definition.go Outdated Show resolved Hide resolved
@sil2100
Copy link
Collaborator

sil2100 commented Aug 30, 2022

One thing to think about: should we maybe provide some fstab default if no fstab is given? Is there a common fstab we can figure out from the gadget yaml and use that as a default?

Co-authored-by: Łukasz Zemczak <lukasz.zemczak@ubuntu.com>
@jawn-smith
Copy link
Contributor Author

jawn-smith commented Aug 30, 2022

One thing to think about: should we maybe provide some fstab default if no fstab is given? Is there a common fstab we can figure out from the gadget yaml and use that as a default?

We have some basic logic in populate_rootfs_contents to handle this. It creates an entry of LABEL=writable / ext4 defaults 0 0 by default. Do you think this is sufficient?

It is very non-trivial to add a default for a struct like this. It is currently a pointer value and is therefore not memory safe to add it as a default in the same way we are assigning default values for strings, booleans, etc.

@sil2100
Copy link
Collaborator

sil2100 commented Sep 2, 2022

I think this is more than enough, thanks for the reminder! Let's merge.

@sil2100 sil2100 merged commit 068a379 into feature/classic-image-redesign Sep 2, 2022
@upils upils deleted the fstab branch September 18, 2023 07:52
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