-
Notifications
You must be signed in to change notification settings - Fork 62
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
Package e2fsprogs with ubuntu-image and select the appropriate conf at runtime #170
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #170 +/- ##
==========================================
- Coverage 94.27% 84.50% -9.77%
==========================================
Files 16 16
Lines 3284 3324 +40
==========================================
- Hits 3096 2809 -287
- Misses 120 439 +319
- Partials 68 76 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
e4959c5
to
fc630bf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you confirmed that everything works as expected when this is built as a snap on a clean environment? If yes, then changes look good. I have some naming-convention change propositions inline, but those are minor really. If you change them, feel free to merge it anyway.
Before doing extending tests I wanted a review from @alfonsosanchezbeato to make sure this is coherent with what we discussed and there is no major flaw. Since this is the third solution (after the rejected SRU on e2fsprogs and the rejected PR on snapd) I implement, I don't want to waste any more time on this if something is blocking. Especially in collect-mkfs-confs.sh I build a list of series with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some minor comments regarding the script that pulls the configuration, but otherwise looks very nice, thanks!
e88e34f
to
59d37df
Compare
@alfonsosanchezbeato thanks for the review. I fixed all of your comments. My main pain point with this solution is that collecting the mkfs configurations is rather slow (need to download a bunch of debs and extract one, per series). So at some point if this is too much of a problem I may look for an alternative solution. I will now run some more extensive manual tests and will add them in the spread tests once #165 is merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the changes!
Regarding the time it takes to download, a possible solution would be to actually store the configurations and keep a small text db pointing to the deb version it was grabed from. And then check with rmadison
in each build if there has been some change in the packages, and error out in that case. As anyway changes will happen very rarely. But not sure how annoying will this be to implement.
I tested the resulting snap (after locally rebasing on #165), ran it on Lunar and Focal and confirmed the resulting filesystem for the rootfs does not use the orphan_file feature (and thus use the custom mkfs configuration associated to the target system). @sil2100 I you have time to test, let me know what you think about the build time. As mentioned by alfonso these confs may change rarely (fetching them at build time is more of a future-proof solution) so we could store them and only check if we miss some (because a new series is out) or if some are outdated. |
59d37df
to
8d4cdde
Compare
0070ee4
to
79c87e7
Compare
@alfonsosanchezbeato would you mind reviewing it again please? I decided to rework it so the mkfs configurations are versioned in the project and I check in CI and at build time that it is up to date. The check is much quicker than actually downloading the configurations, so the build time is not too much impacted. I decided to consider the configuration was the same for every archs so I only get configurations for amd64. This is simpler but might prove wrong at some point. Let me know if you think this fine or not. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Signed-off-by: Paul Mars <[email protected]>
…utdated Signed-off-by: Paul Mars <[email protected]>
Signed-off-by: Paul Mars <[email protected]>
Signed-off-by: Paul Mars <[email protected]>
Signed-off-by: Paul Mars <[email protected]>
Signed-off-by: Paul Mars <[email protected]>
Signed-off-by: Paul Mars <[email protected]>
Signed-off-by: Paul Mars <[email protected]>
Signed-off-by: Paul Mars <[email protected]>
Signed-off-by: Paul Mars <[email protected]>
Signed-off-by: Paul Mars <[email protected]>
Signed-off-by: Paul Mars <[email protected]>
Signed-off-by: Paul Mars <[email protected]>
Signed-off-by: Paul Mars <[email protected]>
Signed-off-by: Paul Mars <[email protected]>
Signed-off-by: Paul Mars <[email protected]>
a6530e8
to
f741401
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, this is looking good. I like that basically the creation of the config only happens manually (+ checks done in CI and at snap build), and that there is a fallback mechanism in case there is no config present. It's still sad that we have to have so much automation over this, but it's certainly much more noninvasive right now.
+1 on merging this!
Signed-off-by: Paul Mars <[email protected]>
Signed-off-by: Paul Mars <[email protected]>
This PR is solving this LP #2028564 bug.
Fixes FR-6018