Skip to content

Commit

Permalink
osbuild: simplify code by removing osbuild.Mounts type
Browse files Browse the repository at this point in the history
The `osbuild.Mounts` type is just an alias to `[]osbuild.Mount`
and it feels like it does not add clarity. From the type it's
not clear if it's a slice or a map (like `osbuild.Devices`). While
it is nice to abstract things away one needs to know it anyway when
constructing the type so this commit removes the type and just
exposes/uses `[]osbuild.Mount` directly.

Another benefit of this is that the pointers used are now clearer
(IMHO) - i.e. `[]Mount` is a slice so no need to indirect it further
via `*Mounts` in the function signatures.
  • Loading branch information
mvo5 authored and achilleas-k committed Jan 17, 2024
1 parent c3a4b38 commit b81b603
Show file tree
Hide file tree
Showing 9 changed files with 18 additions and 22 deletions.
2 changes: 1 addition & 1 deletion pkg/manifest/iso_rootfs.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ func (p *ISORootfsImg) serialize() osbuild.Pipeline {
},
}
copyStageInputs := osbuild.NewPipelineTreeInputs(inputName, p.installerPipeline.Name())
copyStageMounts := &osbuild.Mounts{*osbuild.NewExt4Mount(devName, devName, "/")}
copyStageMounts := []osbuild.Mount{*osbuild.NewExt4Mount(devName, devName, "/")}
copyStage := osbuild.NewCopyStage(copyStageOptions, copyStageInputs, &devices, copyStageMounts)
pipeline.AddStage(copyStage)
return pipeline
Expand Down
2 changes: 1 addition & 1 deletion pkg/manifest/raw_ostree.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ func (p *RawOSTreeImage) serialize() osbuild.Pipeline {
// Find the FS root mount name to use as the destination root
// for the target when copying the boot files.
var fsRootMntName string
for _, mnt := range *bootCopyMounts {
for _, mnt := range bootCopyMounts {
if mnt.Target == "/" {
fsRootMntName = mnt.Name
break
Expand Down
6 changes: 3 additions & 3 deletions pkg/osbuild/bootupd_stage.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,8 @@ func validateBootupdMounts(mounts []Mount) error {
// NewBootupdStage creates a new stage for the org.osbuild.bootupd stage. It
// requires a mount setup of "/", "/boot" and "/boot/efi" right now so that
// bootupd can find and install all required bootloader bits.
func NewBootupdStage(opts *BootupdStageOptions, devices *Devices, mounts *Mounts) (*Stage, error) {
if err := validateBootupdMounts(*mounts); err != nil {
func NewBootupdStage(opts *BootupdStageOptions, devices *Devices, mounts []Mount) (*Stage, error) {
if err := validateBootupdMounts(mounts); err != nil {
return nil, err
}
if err := opts.validate(*devices); err != nil {
Expand All @@ -70,6 +70,6 @@ func NewBootupdStage(opts *BootupdStageOptions, devices *Devices, mounts *Mounts
Type: "org.osbuild.bootupd",
Options: opts,
Devices: *devices,
Mounts: *mounts,
Mounts: mounts,
}, nil
}
10 changes: 5 additions & 5 deletions pkg/osbuild/bootupd_stage_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import (
"github.com/osbuild/images/pkg/osbuild"
)

func makeOsbuildMounts(targets ...string) osbuild.Mounts {
func makeOsbuildMounts(targets ...string) []osbuild.Mount {
var mnts []osbuild.Mount
for _, target := range targets {
mnts = append(mnts, osbuild.Mount{
Expand Down Expand Up @@ -46,7 +46,7 @@ func TestBootupdStageNewHappy(t *testing.T) {
Devices: devices,
Mounts: mounts,
}
stage, err := osbuild.NewBootupdStage(opts, &devices, &mounts)
stage, err := osbuild.NewBootupdStage(opts, &devices, mounts)
require.Nil(t, err)
assert.Equal(t, stage, expectedStage)
}
Expand All @@ -58,7 +58,7 @@ func TestBootupdStageMissingMounts(t *testing.T) {
devices := makeOsbuildDevices("dev-/")
mounts := makeOsbuildMounts("/")

stage, err := osbuild.NewBootupdStage(opts, &devices, &mounts)
stage, err := osbuild.NewBootupdStage(opts, &devices, mounts)
assert.ErrorContains(t, err, "required mounts for bootupd stage [/boot /boot/efi] missing")
require.Nil(t, stage)
}
Expand All @@ -72,7 +72,7 @@ func TestBootupdStageMissingDevice(t *testing.T) {
devices := makeOsbuildDevices("dev-/", "dev-/boot", "dev-/boot/efi")
mounts := makeOsbuildMounts("/", "/boot", "/boot/efi")

stage, err := osbuild.NewBootupdStage(opts, &devices, &mounts)
stage, err := osbuild.NewBootupdStage(opts, &devices, mounts)
assert.ErrorContains(t, err, `cannot find expected device "disk" for bootupd bios option in [dev-/ dev-/boot dev-/boot/efi]`)
require.Nil(t, stage)
}
Expand All @@ -91,7 +91,7 @@ func TestBootupdStageJsonHappy(t *testing.T) {
devices := makeOsbuildDevices("disk", "dev-/", "dev-/boot", "dev-/boot/efi")
mounts := makeOsbuildMounts("/", "/boot", "/boot/efi")

stage, err := osbuild.NewBootupdStage(opts, &devices, &mounts)
stage, err := osbuild.NewBootupdStage(opts, &devices, mounts)
require.Nil(t, err)
stageJson, err := json.MarshalIndent(stage, "", " ")
require.Nil(t, err)
Expand Down
9 changes: 4 additions & 5 deletions pkg/osbuild/copy_stage.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,13 @@ type CopyStagePath struct {

func (CopyStageOptions) isStageOptions() {}

func NewCopyStage(options *CopyStageOptions, inputs Inputs, devices *Devices, mounts *Mounts) *Stage {
func NewCopyStage(options *CopyStageOptions, inputs Inputs, devices *Devices, mounts []Mount) *Stage {
return &Stage{
Type: "org.osbuild.copy",
Options: options,
Inputs: inputs,
Devices: *devices,
Mounts: *mounts,
Mounts: mounts,
}
}

Expand Down Expand Up @@ -60,7 +60,7 @@ func (*CopyStageFilesInputs) isStageInputs() {}
func GenCopyFSTreeOptions(inputName, inputPipeline, filename string, pt *disk.PartitionTable) (
*CopyStageOptions,
*Devices,
*Mounts,
[]Mount,
) {

devices := make(map[string]Device, len(pt.Partitions))
Expand Down Expand Up @@ -118,7 +118,6 @@ func GenCopyFSTreeOptions(inputName, inputPipeline, filename string, pt *disk.Pa
panic("no mount found for the filesystem root")
}

stageMounts := Mounts(mounts)
stageDevices := Devices(devices)

options := CopyStageOptions{
Expand All @@ -130,5 +129,5 @@ func GenCopyFSTreeOptions(inputName, inputPipeline, filename string, pt *disk.Pa
},
}

return &options, &stageDevices, &stageMounts
return &options, &stageDevices, mounts
}
3 changes: 1 addition & 2 deletions pkg/osbuild/copy_stage_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,8 @@ func TestNewCopyStage(t *testing.T) {
Mounts: mounts,
}
// convert to alias types
stageMounts := Mounts(mounts)
stageDevices := Devices(devices)
actualStage := NewCopyStage(&CopyStageOptions{paths}, NewPipelineTreeInputs("tree-input", "input-pipeline"), &stageDevices, &stageMounts)
actualStage := NewCopyStage(&CopyStageOptions{paths}, NewPipelineTreeInputs("tree-input", "input-pipeline"), &stageDevices, mounts)
assert.Equal(t, expectedStage, actualStage)
}

Expand Down
2 changes: 0 additions & 2 deletions pkg/osbuild/mount.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
package osbuild

type Mounts []Mount

type Mount struct {
Name string `json:"name"`
Type string `json:"type"`
Expand Down
2 changes: 1 addition & 1 deletion pkg/osbuild/stage.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ type Stage struct {
Inputs Inputs `json:"inputs,omitempty"`
Options StageOptions `json:"options,omitempty"`
Devices Devices `json:"devices,omitempty"`
Mounts Mounts `json:"mounts,omitempty"`
Mounts []Mount `json:"mounts,omitempty"`
}

// StageOptions specify the operations of a given stage-type.
Expand Down
4 changes: 2 additions & 2 deletions pkg/osbuild/zipl_inst_stage.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ func (ZiplInstStageOptions) isStageOptions() {}

// Return a new zipl.inst stage. The 'disk' parameter must represent the
// (entire) device that contains the /boot partition.
func NewZiplInstStage(options *ZiplInstStageOptions, disk *Device, devices *Devices, mounts *Mounts) *Stage {
func NewZiplInstStage(options *ZiplInstStageOptions, disk *Device, devices *Devices, mounts []Mount) *Stage {
// create a new devices map and add the disk to it
devmap := map[string]Device(*devices)
devmap["disk"] = *disk
Expand All @@ -26,7 +26,7 @@ func NewZiplInstStage(options *ZiplInstStageOptions, disk *Device, devices *Devi
Type: "org.osbuild.zipl.inst",
Options: options,
Devices: ziplDevices,
Mounts: *mounts,
Mounts: mounts,
}
}

Expand Down

0 comments on commit b81b603

Please sign in to comment.