many: pipeline mounts

[supakeen]

Provide machinery to set default mounts and devices on pipelines. Any stage added through .AddStage(s) will be given these options.

This mostly saves (currently) in cases where we need to pass mounts every time.

Perhaps we want to be stricter?

---

In the future I might want to use this in the os pipeline; or a new pipeline, to invert the order of operation so that we first create the image and then write to it; similar to what we currently do for the bootc pipeline(s) but that's for later, it does give some background.

[achilleas-k]

to invert the order of operation so that we first create the image and then write to it

Not the topic here, but I'm not fully onboard with this just yet. The current order has some benefits, like exporting the OS pipeline result for inspection. And there's also the potential future benefit of reproducibility with mkfs.ext4 -d, which I'd love to explore at some point.

[achilleas-k]

Generally a good idea. Some comments below about naming and behaviour details.
I initially missed that the way to opt out of adding stages without applying the defaults is to append to the pipeline's stages directly. That's a bit messy. Not a huge problem, we do similar things when we want to prepend stages, but it might be nicer to make the Pipeline API more useful by having methods that do these things (appending stages, prepending stages, adding with or without defaults, etc) in a consistent way with sanity checking so that we don't have to be appending elements to slices directly.

I'd be curious to see how many stages need opting out and how much trouble/code this saves us in the end. It might be interesting to consider alternatives, like a little closure at the top of the pipeline generator that does the same work, e.g.:

func (p *SomePipeline) serialize() (osbuild.Pipeline, error) {

	...

	mounts := ...  // make pipeline mounts
	devices := ...  // make pipeline devices
	func addStageWithDefaults(stage *Stage) {
		stage.Mounts = mounts
		stage.Devices = devices
		pipeline.AddStage(stage)
	}

	...

	addStageWithDefaults(osbuild.NewWhateverStage(...))

}

or adding an extra method to the pipeline for adding stages with mounts and devices:

func (p *SomePipeline) serialize() (osbuild.Pipeline, error) {

	...

	mounts := ...  // make pipeline mounts
	devices := ...  // make pipeline devices

	pipeline.AddStageWithMounts(osbuild.NewWhateverStage(...), mounts, devices)
}

[achilleas-k]

Given the description of the behaviour in the commit message, I think the term default doesn't work here. Perhaps base or just mounts? I was thinking piplineMounts, but since they're a field of the pipeline struct, just pipeline.mounts will do.

My thinking is that the term defaultMounts could imply that defining any other set of mounts on a stage overrides them, instead of appending to them. But that's not the case. They're global pipeline mounts, for all stages, unconditionally.

[supakeen]

Just mounts / devices is probably the better name here.

[achilleas-k]

Same comment about naming.

[achilleas-k]

I'm not sure this is the correct behaviour here. I think if a stage defines a device with the same name as the global pipeline devices, it might make sense for that to be an error. If we really want the ability to override the device configuration for a stage by adding a device with the same name, we should document it in the docstrings, both here and in the method where the pipeline devices are defined.

[supakeen]

Let's make it error now. If we get a use case where we would need this we can cross the bridge then.

[supakeen]

Generally a good idea. Some comments below about naming and behaviour details. I initially missed that the way to opt out of adding stages without applying the defaults is to append to the pipeline's stages directly. That's a bit messy. Not a huge problem, we do similar things when we want to prepend stages, but it might be nicer to make the Pipeline API more useful by having methods that do these things (appending stages, prepending stages, adding with or without defaults, etc) in a consistent way with sanity checking so that we don't have to be appending elements to slices directly.

I'd be curious to see how many stages need opting out and how much trouble/code this saves us in the end. It might be interesting to consider alternatives, like a little closure at the top of the pipeline generator that does the same work, e.g.:

func (p *SomePipeline) serialize() (osbuild.Pipeline, error) {

	...

	mounts := ...  // make pipeline mounts
	devices := ...  // make pipeline devices
	func addStageWithDefaults(stage *Stage) {
		stage.Mounts = mounts
		stage.Devices = devices
		pipeline.AddStage(stage)
	}

	...

	addStageWithDefaults(osbuild.NewWhateverStage(...))

}

or adding an extra method to the pipeline for adding stages with mounts and devices:

func (p *SomePipeline) serialize() (osbuild.Pipeline, error) {

	...

	mounts := ...  // make pipeline mounts
	devices := ...  // make pipeline devices

	pipeline.AddStageWithMounts(osbuild.NewWhateverStage(...), mounts, devices)
}

I think the little closure mostly makes sense because there currently aren't any other functions provided to add with/without mounts/devices. I was quite unsure about doing those because it's a bit opinionated and there was a good opt out possibility by appending to stages directly.

I'll do an AddStageWithMountsDevices instead and see how that looks 🙂, it should be the non-default behavior at least.

[supakeen]

Now has AddStageDirect instead of flipping the behavior the default behavior is to append pipeline mounts/devices and when *Direct is used they're bypassed.

We also now panic on adding a stage with a device that's pre-existing. This avoids changing like 250 call sites.

Writing all of this however made me realize that it's /probably/ better to rewrite the entire stage adding mechanics to get rid of all the 'must be in the correct order' and to allow better control over everything so I'll close this and think about that.