RHEL-14922: accept a config blob alongside the "changes" slice when committing #20657

nalind · 2023-11-10T21:56:03Z

Does this PR introduce a user-facing change?

`podman container commit` now features a `--config` option which accepts a filename containing a JSON-encoded container configuration to be merged in to the newly-created image.

Fixes #13770.

Luap99

just some drive by comments

Luap99 · 2023-11-13T13:14:16Z

pkg/bindings/containers/types.go

@@ -29,6 +29,7 @@ type LogOptions struct {
 type CommitOptions struct {
 	Author  *string
 	Changes []string
+	Config  []byte


I think it would make more sense to accepts an io.Reader here instead.

Had to make it a *io.Reader to make the generator happy.

Yeah I think the generator could use some improvements, IMO a pointer to a interface makes things harder to use and more likely to cause nil derefs.
Anyway this shouldn't really block your work.

Luap99 · 2023-11-13T13:15:29Z

pkg/domain/entities/containers.go

@@ -164,6 +164,7 @@ type ContainerStatReport struct {
 type CommitOptions struct {
 	Author         string
 	Changes        []string
+	Config         []byte


also io.Reader here.

Had to make it a *io.Reader to make the generator happy.

Luap99 · 2023-11-13T13:17:26Z

pkg/bindings/containers/commit.go

+	if config := params.Get("config"); config != "" {
+		requestBody = strings.NewReader(config)
+		params.Del("config")
+	}


you can add schema:"-" to the type then it will not be added via ToParams()

Ah, thanks, I wouldn't have guessed that.

pkg/api/handlers/libpod/images.go

nalind · 2023-11-17T20:25:39Z

/override Build Each Commit

openshift-ci · 2023-11-17T20:25:43Z

@nalind: /override requires failed status contexts, check run or a prowjob name to operate on.
The following unknown contexts/checkruns were given:

Build
Commit
Each

Only the following failed contexts/checkruns were expected:

Artifacts
Build Each Commit
Total Success
Verify Win Installer Build
machine-hyperv podman windows rootless host sqlite
machine-wsl podman windows rootless host sqlite
tide

If you are trying to override a checkrun that has a space in it, you must put a double quote on the context.

In response to this:

/override Build Each Commit

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

nalind · 2023-11-17T20:26:21Z

/override "Build Each Commit"
... because the vendoring commit isn't going to be in the final version of this PR.

openshift-ci · 2023-11-17T20:26:26Z

@nalind: Overrode contexts on behalf of nalind: Build Each Commit

In response to this:

/override "Build Each Commit"
... because the vendoring commit isn't going to be in the final version of this PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

nalind · 2023-11-18T00:54:54Z

/retitle RHEL-14922: accept a config blob alongside the "changes" slice when committing

mheon · 2023-11-20T19:56:15Z

pkg/domain/entities/containers.go

@@ -164,6 +164,7 @@ type ContainerStatReport struct {
 type CommitOptions struct {
 	Author         string
 	Changes        []string
+	Config         io.Reader


This struct is used for the REST API as well - will this break given that io.Reader can't really be serialized over the network?

Changed it back to a []byte.

packit-as-a-service · 2023-11-28T17:28:18Z

Ephemeral COPR build failed. @containers/packit-build please check.

rhatdan · 2023-11-29T12:00:22Z

LGTM
@mheon @vrothberg PTAL

vrothberg · 2023-11-29T12:15:36Z

docs/source/markdown/podman-commit.1.md

@@ -36,6 +36,11 @@ Apply the following possible instructions to the created image:

 Can be set multiple times.

+#### **--config**=*ConfigBlobFile*
+
+Merge the JSON-encoded container configuration from the specified file into the configuration for the image


Can we point users to a specification of the configuration?

~~It varies between OCI and Docker image formats.~~ I don't think there's a stable location to point to for the Docker config blob format (the engine API documentation is close, but it includes the API version number) ~~, and the OCI version is missing some interesting fields~~. If it wasn't being exercised in tests that I'm about to add, I could pretty easily be talked into dropping the option.
Actually, maybe even after I add the tests to the PR....

Well, as written, it's a JSON-encoded copy of the structure defined at https://github.com/containers/image/blob/main/manifest/docker_schema2.go#L67. Would a pointer to that be useful enough to someone who hasn't spent time staring at that code at some point already?

Great idea, thanks @nalind

Okay, added. Pointed at the currently-newest tag instead of main in case the line number changes.

…tting Use ParseUserNamespace instead of ParseNamespace to parse a passed-in user namespace setting. Signed-off-by: Nalin Dahyabhai <[email protected]>

When committing containers to create new images, accept a container config blob being passed in the body of the API request by adding a Config field to our API structures. Populate it from the body of requests that we receive, and use its contents as the body of requests that we make. Make the libpod commit endpoint split changes values at newlines, just like the compat endpoint does. Pass both the config blob and the "changes" slice to buildah's Commit() API, so that it can handle cases where they overlap or conflict. Signed-off-by: Nalin Dahyabhai <[email protected]>

nalind · 2023-11-30T14:02:09Z

Changed a line in the description of the -v flag for build to match what buildah's doc says about it, which I think removes a bit of ambiguity.

Be specific that the `-v` flag only affects RUN instructions. The previous wording left it ambiguous, and people might have concluded that it applied to ADD and COPY as well. Signed-off-by: Nalin Dahyabhai <[email protected]>

rhatdan · 2023-12-01T21:04:43Z

/approve
/lgtm

openshift-ci · 2023-12-01T21:04:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nalind, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

nalind · 2024-02-07T14:24:35Z

/cherry-pick v4.9.1-rhel

openshift-cherrypick-robot · 2024-02-07T14:25:36Z

@nalind: new pull request created: #21547

In response to this:

/cherry-pick v4.9.1-rhel

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

nalind · 2024-02-07T20:22:48Z

/cherry-pick v4.9

openshift-cherrypick-robot · 2024-02-07T20:23:44Z

@nalind: new pull request created: #21549

In response to this:

/cherry-pick v4.9

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note labels Nov 10, 2023

github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Nov 10, 2023

Luap99 reviewed Nov 13, 2023

View reviewed changes

nalind force-pushed the commit-config branch 4 times, most recently from 88dcb4f to 130fe7f Compare November 17, 2023 20:00

nalind force-pushed the commit-config branch 2 times, most recently from a011559 to 8b7478b Compare November 17, 2023 23:04

openshift-ci bot changed the title ~~WIP: RHEL-14922: accept a config blob alongside the "changes" slice when committing~~ RHEL-14922: accept a config blob alongside the "changes" slice when committing Nov 18, 2023

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 18, 2023

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 19, 2023

nalind force-pushed the commit-config branch from 8b7478b to ea7b1de Compare November 20, 2023 16:19

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 20, 2023

mheon reviewed Nov 20, 2023

View reviewed changes

nalind force-pushed the commit-config branch 4 times, most recently from 77c513a to 08c51ea Compare November 28, 2023 16:31

vrothberg reviewed Nov 29, 2023

View reviewed changes

nalind force-pushed the commit-config branch 2 times, most recently from bd40243 to 7289285 Compare November 29, 2023 19:11

nalind added 2 commits November 30, 2023 09:00

container create: use ParseUserNamespace to parse a user namespace se…

e197cf5

…tting Use ParseUserNamespace instead of ParseNamespace to parse a passed-in user namespace setting. Signed-off-by: Nalin Dahyabhai <[email protected]>

nalind force-pushed the commit-config branch from 7289285 to dbbe72e Compare November 30, 2023 14:00

@@option volume.image: be specific that -v only affects RUN

fa0aa91

Be specific that the `-v` flag only affects RUN instructions. The previous wording left it ambiguous, and people might have concluded that it applied to ADD and COPY as well. Signed-off-by: Nalin Dahyabhai <[email protected]>

nalind force-pushed the commit-config branch from dbbe72e to fa0aa91 Compare November 30, 2023 14:05

openshift-ci bot assigned rhatdan Dec 1, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 1, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 1, 2023

openshift-merge-bot bot merged commit c479628 into containers:main Dec 1, 2023
93 checks passed

nalind deleted the commit-config branch December 1, 2023 21:11

matejvasek mentioned this pull request Dec 4, 2023

s2i does not update Cmd after assemble with podman service openshift/source-to-image#1082

Open

openshift-cherrypick-robot mentioned this pull request Feb 7, 2024

[v4.9.1-rhel] RHEL-14922: accept a config blob alongside the "changes" slice when committing #21547

Merged

openshift-cherrypick-robot mentioned this pull request Feb 7, 2024

[v4.9] RHEL-14922: accept a config blob alongside the "changes" slice when committing #21549

Merged

stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label May 8, 2024

stale-locking-app bot locked as resolved and limited conversation to collaborators May 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RHEL-14922: accept a config blob alongside the "changes" slice when committing #20657

RHEL-14922: accept a config blob alongside the "changes" slice when committing #20657

nalind commented Nov 10, 2023 •

edited

Loading

Luap99 left a comment

Luap99 Nov 13, 2023

nalind Nov 16, 2023

Luap99 Nov 17, 2023

Luap99 Nov 13, 2023

nalind Nov 16, 2023

Luap99 Nov 13, 2023

nalind Nov 16, 2023

nalind commented Nov 17, 2023

openshift-ci bot commented Nov 17, 2023

nalind commented Nov 17, 2023

openshift-ci bot commented Nov 17, 2023

nalind commented Nov 18, 2023

mheon Nov 20, 2023

nalind Nov 27, 2023

packit-as-a-service bot commented Nov 28, 2023

rhatdan commented Nov 29, 2023

vrothberg Nov 29, 2023

nalind Nov 29, 2023 •

edited

Loading

nalind Nov 29, 2023

vrothberg Nov 30, 2023

nalind Nov 30, 2023

nalind commented Nov 30, 2023

rhatdan commented Dec 1, 2023

openshift-ci bot commented Dec 1, 2023

nalind commented Feb 7, 2024

openshift-cherrypick-robot commented Feb 7, 2024

nalind commented Feb 7, 2024

openshift-cherrypick-robot commented Feb 7, 2024

RHEL-14922: accept a config blob alongside the "changes" slice when committing #20657

RHEL-14922: accept a config blob alongside the "changes" slice when committing #20657

Conversation

nalind commented Nov 10, 2023 • edited Loading

Does this PR introduce a user-facing change?

Luap99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalind commented Nov 17, 2023

openshift-ci bot commented Nov 17, 2023

nalind commented Nov 17, 2023

openshift-ci bot commented Nov 17, 2023

nalind commented Nov 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

packit-as-a-service bot commented Nov 28, 2023

rhatdan commented Nov 29, 2023

Choose a reason for hiding this comment

nalind Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalind commented Nov 30, 2023

rhatdan commented Dec 1, 2023

openshift-ci bot commented Dec 1, 2023

nalind commented Feb 7, 2024

openshift-cherrypick-robot commented Feb 7, 2024

nalind commented Feb 7, 2024

openshift-cherrypick-robot commented Feb 7, 2024

nalind commented Nov 10, 2023 •

edited

Loading

nalind Nov 29, 2023 •

edited

Loading