-
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
FR-5649 - Distinguish build and target components #160
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #160 +/- ##
==========================================
- Coverage 90.52% 90.07% -0.46%
==========================================
Files 13 13
Lines 2628 2629 +1
==========================================
- Hits 2379 2368 -11
- Misses 216 229 +13
+ Partials 33 32 -1 ☔ View full report in Codecov by Sentry. |
709b898
to
f0c2efb
Compare
f0c2efb
to
d7ae984
Compare
d7ae984
to
65f518d
Compare
65f518d
to
465bf24
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.
This is good, thank you! Inline comments are mainly just whitespace issues and ideas for future improvement (but for a separate PR). I think we can merge it like this or merge instantly after whitespace comments are addressed - both are fine.
@@ -39,7 +39,7 @@ type Gadget struct { | |||
|
|||
// Rootfs defines the rootfs section of the image definition file | |||
type Rootfs struct { | |||
Components []string `yaml:"components" json:"Components,omitempty"` | |||
Components []string `yaml:"components" json:"Components,omitempty" default:"main,restricted"` |
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.
Whitespace correction to match the spacing of other rows default:
column.
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.
Good catch. Fixed.
strings.Join(imageDef.Rootfs.Components, " "), | ||
), | ||
}, | ||
func generatePocketList(series string, components []string, mirror string, securityMirror string, pocket string) []string { |
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.
Oh yes this is much better.
@@ -517,6 +520,21 @@ func (stateMachine *StateMachine) prepareGadgetTree() error { | |||
return nil | |||
} | |||
|
|||
// fixHostname set fresh hostname since debootstrap copies /etc/hostname from build environment | |||
func (stateMachine *StateMachine) fixHostname() error { |
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.
hm, this is fine, but I think restructuring like this is best done as part of separate PRs. Also, for the future I'd like us to maybe have a way of structuring which methods in the StateMachine
struct are methods for states and which are 'helper' methods like this. Ideally (but we're not doing that right now), those state methods would be easily distinguishable by either them being all in one place and helpers in a different file, or by some naming scheme.
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 agree, I thought the same this week when solving the broken resume feature.
|
||
// overwriteSourcesList replaces /etc/apt/sources.list with the given list of entries | ||
// This function will truncate the existing file. | ||
func (stateMachine *StateMachine) overwriteSourcesList(aptSources []string) error { |
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.
Another helper I'd like us to distinguish as a helper.
t.Cleanup(restoreMock) | ||
} | ||
|
||
err = stateMachine.customizeSourcesList() |
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 am surprised that this doesn't satisfy codecov above for the TargetPocketList()
coverage.
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.
Me too. I suspect this is because this call of the function is outside the image_definition package and the coverage is not registered.
465bf24
to
73634aa
Compare
No description provided.