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

snap-bootstrap: refactor partition creation #7958

Merged
merged 3 commits into from Jan 8, 2020

Conversation

cmatsuoka
Copy link
Member

Refactor filesystem creation, deployment and mounting to act on a single
partition instead of a slice of partitions.

Based on Michael Vogt's work in PR #7723.

Signed-off-by: Claudio Matsuoka claudio.matsuoka@canonical.com

Refactor filesystem creation, deployment and mounting to act on a single
partition instead of a slice of partitions.

Based on Michael Vogt's work in PR snapcore#7723.

Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
@cmatsuoka cmatsuoka added the UC20 label Jan 6, 2020
Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Looks nice, thanks! Makes the creation more atomic, I wonder if we can later use something like the partition (gpt) flags to mark each fully successful partition as pending and done (but that is certainly a followup :)

if err := partition.DeployContent(created, gadgetRoot); err != nil {
return err
}
for _, part := range created {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, thanks for this!

err := partition.MakeFilesystem(part)
c.Assert(err, IsNil)
c.Assert(s.mockMkfsVfat.Calls(), HasLen, 1)
c.Assert(s.mockMkfsExt4.Calls(), HasLen, i)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a small comment here, when I skimmed over this (admittedly in the early morning) I missed that the first line has a "1" and the second a "i". Maybe something like // single fat partition is created first, then 2 ext4 partitions. But not a blocker :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I think the difference is much more noticeable in the terminal font. Adding the comment, and also changing the variable to n to prevent misreadings.

Add a comment to the cumulative filesystem creation test and change
the iterator variable name to prevent confusion between 1 and i.

Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
@codecov-io
Copy link

codecov-io commented Jan 7, 2020

Codecov Report

Merging #7958 into master will increase coverage by 0.19%.
The diff coverage is 48.48%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7958      +/-   ##
==========================================
+ Coverage   80.86%   81.05%   +0.19%     
==========================================
  Files         698      698              
  Lines       54986    55811     +825     
==========================================
+ Hits        44462    45240     +778     
- Misses       7113     7135      +22     
- Partials     3411     3436      +25
Impacted Files Coverage Δ
cmd/snap-bootstrap/bootstrap/bootstrap.go 60.65% <0%> (-1.02%) ⬇️
cmd/snap-bootstrap/partition/deploy.go 63.26% <60%> (ø) ⬆️
cmd/snap-bootstrap/partition/mkfs.go 86.66% <66.66%> (+0.95%) ⬆️
osutil/synctree.go 76.82% <0%> (-2.44%) ⬇️
boot/kernel_os.go 97.89% <0%> (-2.11%) ⬇️
cmd/snap/cmd_snap_op.go 80.32% <0%> (-0.34%) ⬇️
daemon/api.go 79.25% <0%> (-0.02%) ⬇️
overlord/snapstate/policy/policy.go 100% <0%> (ø) ⬆️
overlord/snapstate/policy/errors.go 0% <0%> (ø) ⬆️
cmd/snap/cmd_sign.go 61.76% <0%> (+0.22%) ⬆️
... and 11 more

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 529087e...7d78c6a. Read the comment docs.

Copy link
Collaborator

@bboozzoo bboozzoo left a comment

Choose a reason for hiding this comment

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

Nice cleanup!

@cmatsuoka cmatsuoka merged commit 7649fd2 into snapcore:master Jan 8, 2020
@cmatsuoka cmatsuoka deleted the core20-bootstrap-refactor-1 branch June 25, 2020 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants