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

osbuild: simplify containers_input.go #349

Merged

Conversation

mvo5
Copy link
Contributor

@mvo5 mvo5 commented Jan 5, 2024

[only the second commit is new in this PR]

While writing tests to ensure the that the ContainerDeployInputs
generates the correct json I noticed that this was more involved
than I had hoped and it seems there are some indirections in the
code that may not be necessary. I removed them and IMHHO the code
is now a bit more direct and easier to read.

Feedback very welcome!

Build on top of #348 (as I wanted the unit tests from it).

@supakeen
Copy link
Member

supakeen commented Jan 5, 2024

It's an interesting change. As far as I know it is idiomatic Go to custom type things like these and have an interface for if you ever want to put other stuff in there. However, that only needs to be done when we have other things.

@achilleas-k
Copy link
Member

There's a lot of stupid stuff in the osbuild package in this repo and most of it is from (sometimes, if not often, incorrect) mirroring of the osbuild schema and its flexibility. In this case, the reference for containers input is an interface because inputs in osbuild can (but in the case of containers don't) have multiple schemas. It's also possible for stages to define extra properties for their inputs, though I think we don't do this (though all inputs in osbuild have additionalProperties set to True. For example, iirc, at one point in the development stage of the manifest list feature, the option to include the manifest list was an input option on the container input but only defined in the skopeo stage.

Anyway. I agree with this change. No need to complicate things unnecessarily. Let's merge this.
We need more simplifications like this. Maybe we can make a plan and do it all in one go instead of chipping away at it bit by bit.

achilleas-k
achilleas-k previously approved these changes Jan 5, 2024
Copy link
Member

@achilleas-k achilleas-k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Let's add it to the queue after #348 is added.

@achilleas-k achilleas-k marked this pull request as ready for review January 5, 2024 13:07
@achilleas-k achilleas-k added this pull request to the merge queue Jan 5, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 5, 2024
@achilleas-k achilleas-k force-pushed the osbuild/container-deploy-less-interfaces branch from 1f2de0e to 41fae22 Compare January 5, 2024 13:11
@achilleas-k
Copy link
Member

We need to drop the first commit here since the stage filename has been renamed in the other PR that just got merged.

While writing tests to ensure the that the `ContainerDeployInputs`
generates the correct json I noticed that this was more involved
than I had hoped and it seems there are some indirections in the
code that may not be necessary. I removed them and IMHHO the code
is now a bit more direct and easier to read.
@achilleas-k achilleas-k added this pull request to the merge queue Jan 5, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 5, 2024
@supakeen supakeen added this pull request to the merge queue Jan 5, 2024
Merged via the queue into osbuild:main with commit dae9496 Jan 5, 2024
10 checks passed
@mvo5 mvo5 deleted the osbuild/container-deploy-less-interfaces branch January 5, 2024 15:57
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.

3 participants