Skip to content

client/container_exec: Wrap options and result#51262

Merged
thaJeztah merged 1 commit intomoby:masterfrom
vvoland:client-containerexec-opts
Oct 22, 2025
Merged

client/container_exec: Wrap options and result#51262
thaJeztah merged 1 commit intomoby:masterfrom
vvoland:client-containerexec-opts

Conversation

@vvoland
Copy link
Contributor

@vvoland vvoland commented Oct 22, 2025

`client`: `ContainerExec...` methods were renamed to `Exec...`.

@vvoland vvoland added this to the 29.0.0 milestone Oct 22, 2025
@vvoland vvoland self-assigned this Oct 22, 2025
@vvoland vvoland force-pushed the client-containerexec-opts branch 2 times, most recently from c0b1502 to cecdf29 Compare October 22, 2025 11:12
@vvoland vvoland added the kind/refactor PR's that refactor, or clean-up code label Oct 22, 2025
@vvoland vvoland force-pushed the client-containerexec-opts branch from cecdf29 to c54ece9 Compare October 22, 2025 11:46
Comment on lines -24 to 25
_, err = client.ContainerExecCreate(context.Background(), "container_id", ExecCreateOptions{})
_, err = client.ContainerExecCreate(context.Background(), "container_id", ContainerExecCreateOptions{})
assert.Check(t, is.ErrorType(err, cerrdefs.IsInternal))
Copy link
Member

Choose a reason for hiding this comment

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

Was on the fence on renaming ExecCreateOptions to ContainerExecCreateOptions - considering if we should go the reverse, and make Exec its own thing (not ContainerXXX); WDYT?

Copy link
Contributor Author

@vvoland vvoland Oct 22, 2025

Choose a reason for hiding this comment

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

Yeah I think it could work too.. But whatever we choose we should match the prefix of the option/result struct with the operation name.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, perhaps ExecCreate instead of ContainerExecCreate would work; it's shorter as well, and matches that we have the ExecClient interface defined .. which was slightly to split things a bit.

Let me know what you think

@vvoland vvoland force-pushed the client-containerexec-opts branch 2 times, most recently from c9f6be6 to 0e78a5c Compare October 22, 2025 12:27
@vvoland vvoland added impact/changelog impact/go-sdk Noteworthy (compatibility changes) in the Go SDK labels Oct 22, 2025
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Member

Oh! We should probably squash the commits, as there's some types that were renamed, and back; let me do so (and set the /verify-only label

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
@thaJeztah thaJeztah force-pushed the client-containerexec-opts branch from 0e78a5c to 94ab385 Compare October 22, 2025 20:49
@thaJeztah thaJeztah merged commit f2412e7 into moby:master Oct 22, 2025
54 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/dependencies ci/validate-only impact/changelog impact/go-sdk Noteworthy (compatibility changes) in the Go SDK kind/refactor PR's that refactor, or clean-up code module/client

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants