RHEL-14922: accept a config blob alongside the "changes" slice when committing by nalind · Pull Request #20657 · containers/podman

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

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

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

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.

Details

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

Details

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

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

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 <nalin@redhat.com>

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 <nalin@redhat.com>

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 <nalin@redhat.com>

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

Details

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

Details

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

Details

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 <nalin@redhat.com>

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 <nalin@redhat.com>

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

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

Conversation

nalind commented Nov 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Does this PR introduce a user-facing change?

Uh oh!

Luap99 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nalind commented Nov 17, 2023

Uh oh!

openshift-ci bot commented Nov 17, 2023

Uh oh!

nalind commented Nov 17, 2023

Uh oh!

openshift-ci bot commented Nov 17, 2023

Uh oh!

nalind commented Nov 18, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

rhatdan commented Nov 29, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nalind Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nalind commented Nov 30, 2023

Uh oh!

rhatdan commented Dec 1, 2023

Uh oh!

openshift-ci bot commented Dec 1, 2023

Uh oh!

nalind commented Feb 7, 2024

Uh oh!

openshift-cherrypick-robot commented Feb 7, 2024

Uh oh!

nalind commented Feb 7, 2024

Uh oh!

openshift-cherrypick-robot commented Feb 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

nalind commented Nov 10, 2023 •

edited

Loading

nalind Nov 29, 2023 •

edited

Loading