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.

Sign up for GitHub

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

Jump to bottom

Add container resolver for otk: otk-resolve-containers (COMPOSER-2293) #936

Merged
merged 7 commits into from
Sep 18, 2024
Merged

Add container resolver for otk: otk-resolve-containers (COMPOSER-2293) #936

merged 7 commits into from
Sep 18, 2024

Conversation

achilleas-k
Copy link
Member

New cmd for resolving containers for otk.

The resolver is a thin wrapper around the container.Resolver. It can handle multiple containers for the same architecture in one call.

supakeen
supakeen reviewed Sep 16, 2024
View reviewed changes
Copy link
Member

@supakeen supakeen left a comment

Choose a reason for hiding this comment

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

Going to leave the same comment as on #935: what about the const. We can resolve both in one go there. Otherwise very nice.

mvo5
mvo5 reviewed Sep 17, 2024
View reviewed changes
Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Thanks! This looks nice, ideally we would apply the outcome of the const discusison here and there are some ideas/suggestions inline for your considerations, mostly about spelling out the inputs/outputs slightly more explicitly

cmd/otk-resolve-containers/main_test.go Outdated Show resolved Hide resolved
cmd/otk-resolve-containers/main_test.go Outdated Show resolved Hide resolved
cmd/otk-resolve-containers/main_test.go Show resolved Hide resolved
cmd/otk-resolve-containers/main_test.go Show resolved Hide resolved
@achilleas-k
Copy link
Member Author

Going to leave the same comment as on #935: what about the const. We can resolve both in one go there. Otherwise very nice.

Done! Moved output under const and also added an extra level, containers, in case we need to add more to the output at some point, because the previous top-level object (the one under .tree.const) is an array.

mvo5
mvo5 previously approved these changes Sep 18, 2024
View reviewed changes
Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Thank you! This is nice and I really like the detail that pkg/containers/spec.go:Spec is not directly exposed as we have now more control.

There are some testing nitpicks/suggestions inline but as they only affect testing (which is already comprehensive) and are very opinionated (not neccessarily in a good way, sorry for that!) - the testing is very nice and comprehensive (please don't get me wrong here). If you like the ideas I am happy to do a followup, if you don't I will just be silent :)

The only thing that we might consider is the coment/question about LocalStorage (but given that most of the times I think it will be false/not visible(?) it should cause no harm even if we merge as is).

cmd/otk-resolve-containers/main_test.go Show resolved Hide resolved
cmd/otk-resolve-containers/main_test.go Outdated Show resolved Hide resolved
cmd/otk-resolve-containers/main_test.go Outdated Show resolved Hide resolved
internal/testregistry/testregistry.go Show resolved Hide resolved
cmd/otk-resolve-containers/main.go Outdated Show resolved Hide resolved
supakeen
supakeen previously approved these changes Sep 18, 2024
View reviewed changes
Copy link
Member

@supakeen supakeen left a comment

Choose a reason for hiding this comment

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

Needs the other comments resolved, awesome.

@achilleas-k achilleas-k dismissed stale reviews from supakeen and mvo5 via 62cea3c September 18, 2024 11:08
mvo5
mvo5 previously approved these changes Sep 18, 2024
View reviewed changes
Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Thank you

@achilleas-k
cmd: new otk external: otk-resolve-containers
eb41b1a 
The resolver is a thin wrapper around the container.Resolver.
It can handle multiple containers for the same architecture in one call.
@achilleas-k
pkg/container: move test registry implementation out of tests
3cccb9e 
Move the toy implementation of a container registry out of the
container_test package and into internal/testregistry so it can be
reused across subpackages of the repository.
@achilleas-k
cmd/otk-resolve-containers: add resolver test
519f7d3
@achilleas-k
cmd/otk-resolve-containers: repeat tests with different arch
6a11176 
Run the same tests with the architecture set to ppc64le.
@achilleas-k
cmd/otk-resolve-containers: add error tests
414dc71
@achilleas-k
testregistry: move rootLayer to testregistry and export
b7c4180 
Move the root layer const to the test registry so it can be reused for
testing.  Add a comment describing what it represents and how it can be
used.
@achilleas-k
cmd/otk-resolve-containers: add raw JSON test
31d94be 
Add an extra test that generates a raw json string as input and checks
the raw json string from the output.  This test resolves only one
container to make the construction of the output less tedious and avoid
the need to predict and verify the order of the results.
@supakeen supakeen added this pull request to the merge queue Sep 18, 2024
Merged via the queue into osbuild:main with commit 6a842e3 Sep 18, 2024
18 of 19 checks passed
@achilleas-k achilleas-k deleted the otk-external/container-resolver branch September 18, 2024 15:03
@supakeen supakeen mentioned this pull request Sep 18, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@supakeen supakeen supakeen approved these changes

@mvo5 mvo5 mvo5 left review comments

Assignees
No one assigned
Labels
🌟 best practice
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@achilleas-k @mvo5 @supakeen @schutzbot