-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
feat(podman-remote): support kube play build client #24435
base: main
Are you sure you want to change the base?
feat(podman-remote): support kube play build client #24435
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fixomatic-ctrl The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @Luap99, I moved this to draft because I think this PR is too big. Before I try to split it, could you tell me what do you think about the direction I took and you opinion ? Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't looked closely but I don't mind the the size. As long as everything is one feature (and it is) splitting it does not really make reviewing easier as I would still need to read the same code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add Fixes #14527
as part of the commit message and squash the commits.
One thing I do wonder I would assume we can share much more code with the bindings/build code where we assemble the tar context dir but I haven't looked to closely itno the issue you mentioned
Cannot be used, as it has very particular mechanism for any files/folder added after the first one. Therefore I created a TarBuilder struct, to be able to easily manipulate the creation of the tar.
I was hopeful to have some easy support through the github.com/containers/storage/pkg/archive package, has they have similar method implemented, but they do not expose it. (See discussion i opened on their repo moby/moby#48610)
Note were are using containers/storage not moby so if anything can be added to simplify code here we can most likely make it happen if it is useful there
Have some minor style nits, overall code LGTM. Adding bloat_approved. |
(We can debate about whether the bloat is good or not, but I do want to see tests run) |
Fixes containers#14527 Signed-off-by: fixomatic-ctrl <[email protected]>
f8d5ce0
to
d320148
Compare
Done 👍
If we look at the podman/pkg/bindings/images/build.go Line 707 in 043b82e
is very similar (same logic) from the implementation of containers/storage/pkg/archive#TarWithOptions.
The remark is the same between the repositories docker, and the storage as the same methods are exposed for tar interactions. After reading in details the implementation of containers/storage/pkg/archive#TarWithOptions we see that they are using a struct The requirement are to be able to take a mapping string -> string where the key is the source on the filesystem and the value is the target path in the tar. Due to the structure of the tar file of the kube build remote, we need to be able to build with precision the tarball with the right names. I am not confortable enough with the different repository to be able to conduct such change. |
Fixes #14527
Following #24015
Does this PR introduce a user-facing change?
Changes
I can split this PR in smaller chunk if needed.
Yaml Parsing Utility methods
Moving
SplitMultiDocYAML
,getKubeKind
,imageNamePrefix
,getBuildFile
frompkg/domain/infra/abi/play.go
to the utilitypkg/util/kube.go
to be able to access it from the bindingThis is necessary to have a single way of parsing those file between the client and the server, otherwise the client could send contexts that the server may not use.
TarBuilder
The kube play binding will create a tarball and add several folders to a specific place in the tar. The images build have some similar logic, however the
nTar
function they used (bellow)podman/pkg/bindings/images/build.go
Line 703 in 66fa014
Cannot be used, as it has very particular mechanism for any files/folder added after the first one. Therefore I created a TarBuilder struct, to be able to easily manipulate the creation of the tar.
I was hopeful to have some easy support through the
github.com/containers/storage/pkg/archive
package, has they have similar method implemented, but they do not expose it. (See discussion i opened on their repo moby/moby#48610)I had to move
pkg/bindings/images/build_*.go
to thepkg/bindings/internal/util/build_*.go
because importing those from the internal/util was leading to a circular import issue.Adding
Build
andContextDir
to typesIn
pkg/bindings/kube/types.go
adding theBuild
andContextDir
types, to binding can use them.Testing
pkg/bindings/internal/util/tar_builder_test.go
ensure the TarBuilder struct works as expectedpkg/bindings/kube/kube_test.go
ensure client parse properly yaml filestests/e2e/play_build_test.go
ensure podman-remote can use--build
You may now use
./bin/podman-remote kube play --build example.yaml
:)