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-5649 - Distinguish build and target components #160

Merged
merged 8 commits into from
Nov 23, 2023

Conversation

upils
Copy link
Collaborator

@upils upils commented Oct 24, 2023

No description provided.

@codecov
Copy link

codecov bot commented Oct 25, 2023

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (729cd33) 90.52% compared to head (465bf24) 90.07%.

❗ Current head 465bf24 differs from pull request most recent head 53b87ac. Consider uploading reports for the commit 53b87ac to get more accurate results

Files Patch % Lines
internal/imagedefinition/image_definition.go 54.83% 14 Missing ⚠️
internal/statemachine/classic_states.go 86.66% 2 Missing and 2 partials ⚠️
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.
📢 Have feedback on the report? Share it here.

@upils upils force-pushed the distinguish-build-target-components branch 3 times, most recently from 709b898 to f0c2efb Compare October 25, 2023 12:39
@upils upils force-pushed the distinguish-build-target-components branch from f0c2efb to d7ae984 Compare October 25, 2023 12:44
@upils upils requested a review from sil2100 October 25, 2023 12:45
@upils upils marked this pull request as ready for review October 25, 2023 12:45
Base automatically changed from invalid-fstab to main October 26, 2023 07:08
@upils upils force-pushed the distinguish-build-target-components branch from d7ae984 to 65f518d Compare October 26, 2023 07:14
@upils upils force-pushed the distinguish-build-target-components branch from 65f518d to 465bf24 Compare November 21, 2023 16:48
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, 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"`
Copy link
Collaborator

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.

Copy link
Collaborator Author

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 {
Copy link
Collaborator

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.

internal/imagedefinition/image_definition_test.go Outdated Show resolved Hide resolved
@@ -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 {
Copy link
Collaborator

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.

Copy link
Collaborator Author

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 {
Copy link
Collaborator

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()
Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

@upils upils force-pushed the distinguish-build-target-components branch from 465bf24 to 73634aa Compare November 23, 2023 15:25
@upils upils merged commit 3be0649 into main Nov 23, 2023
9 of 10 checks passed
@upils upils deleted the distinguish-build-target-components branch November 23, 2023 15: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
Development

Successfully merging this pull request may close these issues.

2 participants