bootc: add integration test for disk.yaml (HMS-9296) #1848
Conversation
achilleas-k
left a comment
achilleas-k
left a comment
There was a problem hiding this comment.
I think I understand why you're not entirely happy with this, but the general approach is good. The last commit seems to clean things up a bit, but the previous version wasn't bad either.
| require.NoError(t, err) | ||
| } | ||
|
|
||
| func makeUsrBinInstall(t *testing.T, buildDir string) { |
There was a problem hiding this comment.
Thank for catching this as well. I will tweak the name, its indeed confusing.
| output, err := cmd.CombinedOutput() | ||
| if err != nil { | ||
| require.NoError(t, err, string(output)) | ||
| } |
There was a problem hiding this comment.
Out of curiosity why the condition? To avoid string conversion?
There was a problem hiding this comment.
Nice catch, thank you! This was just a silly oversight by me, I will fix it :)
mvo5
force-pushed
the
bootc-build-imgref-test-5
branch
from
f4ebec6 to
0d2ba70
Compare
supakeen
force-pushed
the
bootc-build-imgref-test-5
branch
from
0d2ba70 to
f482e45
Compare
supakeen
force-pushed
the
bootc-build-imgref-test-5
branch
from
f482e45 to
3ce47a1
Compare
thozza
left a comment
thozza
left a comment
There was a problem hiding this comment.
This looks great. I think that it is a reasonable compromise that does the job. 👏
| type Stage struct { | ||
| Type string `json:"type"` | ||
|
|
||
| Inputs map[string]map[string]any `json:"inputs,omitempty"` | ||
| Options map[string]any `json:"options,omitempty"` | ||
| } |
There was a problem hiding this comment.
While looking at this and the implementation in osbuild package, I got an idea that we could define a very basic Unmarshaler that would do basically the transformation to generic types as you do here.
For some interfaces, we could try adding the Unmarshaller to the embedded common types, i.e. (https://github.com/osbuild/images/blob/bb797792fc4007efd2ef4dfaf1e4cabc5b476ea0/pkg/osbuild/input.go#L20C6-L20C17). For other interfaces, we could simply use generic types as you do here.
My point is that IMHO we do not need to add a custom unmarshaller to all stages or inputs, but just to the top-level common structures. However, the outcome may be messier than this single and self-contained file.
pkg/distro/bootc/integration_test.go
Outdated
| assert.Contains(t, manifestJson, `{"type":"org.osbuild.write-device","inputs":{"tree":{"type":"org.osbuild.tree","origin":"org.osbuild.pipeline","references":["name:target"]}}`) | ||
| assert.Equal(t, []string{"target", "build", "image", "qcow2"}, mani.PipelineNames()[:4]) | ||
|
|
||
| stage := mani.Pipelines[2].Stage("org.osbuild.write-device") |
There was a problem hiding this comment.
Nitpick: given that we are starting to use the stage IDs for testing, maybe it would make sense to define those as constants in the osbuild package in stage implementation and use that instead of literal values. However, I do admit that it is a lot of busy work for no apparent benefit, since the test would fail if one makes a typo in the literal value....
mvo5
changed the title
github-merge-queue
bot
removed this pull request from the merge queue due to a conflict with the base branch
github-merge-queue
bot
removed this pull request from the merge queue due to a conflict with the base branch
mvo5
force-pushed
the
bootc-build-imgref-test-5
branch
from
3ce47a1 to
ec1e12c
Compare
|
This is very stange - the commits for this have landed in main (e.g. ec1e12c) but for some reason GH closed this PR with unmerged commits?!? |
It shows to me that there are 0 commits in this PR so I guess the message is just wrong? |
6 participants
[includes https://github.com//pull/1828]
bootc,manifestest: use *json.RawMessage instead of
map[string]
This is an alternative idea to the previous commit, using
json.RawMessage allows us to use the "native" structs at the
expense of a bit more verbosity inside the test itself.
Thanks to Thozza and Achilleas for their suggestions her here!
bootc: convert TestInteratedBuildDiskYAML to use new
manifestest
This commit converts the TestInteratedBuildDiskYAML to use the
new helpers from manifesttest. A bit better then checking for
the raw json but still a bit unsatificatory because of the
disconnect between the osbuild types and the manifesttest types.
manifesttest: add NewManifestFromBytes() and Pipeline/Stage
etc
This commit is a step towards a more realistic manifesttest. It
will unmarshal the json into a similar structure as osbuild.Manifest
would do. Ideally this would not be needed and osbuild would do
it all via
osbuild.NewManifestFromBytes. However because weuse interfaces in osbuild.Stage the unmarshal is not symetric
to the marshal and we cannot unmarshal back without writing a
bunch of code in a custom osbuild.Stage.Unmarshal handler.
Doing that just for testing seems a bit overkill. Its a bit
sad though because it means more duplication and drift.