api/types/container: move container options to client#50897
api/types/container: move container options to client#50897thaJeztah merged 2 commits intomoby:masterfrom
Conversation
| func (cli *Client) ServiceLogs(ctx context.Context, serviceID string, options container.LogsOptions) (io.ReadCloser, error) { | ||
| func (cli *Client) ServiceLogs(ctx context.Context, serviceID string, options ContainerLogsOptions) (io.ReadCloser, error) { |
There was a problem hiding this comment.
Wondering if we should create a separate ServiceLogOptions struct to allow them to diverge, but also if we want to do the functional options in stages.
There was a problem hiding this comment.
+1, separate service log options. Can be follow-up PR though if you want.
|
|
||
| func (c *containerAdapter) logs(ctx context.Context, options api.LogSubscriptionOptions) (<-chan *backend.LogMessage, error) { | ||
| apiOptions := &containertypes.LogsOptions{ | ||
| apiOptions := &backend.ContainerLogsOptions{ |
There was a problem hiding this comment.
Same for here; would there be a reason to split to service / container?
thaJeztah
force-pushed
the
move_container_options
branch
from
c039cdc to
94e40ee
Compare
thaJeztah
force-pushed
the
move_container_options
branch
from
94e40ee to
8b6f498
Compare
thaJeztah
added
area/api
thaJeztah
force-pushed
the
move_container_options
branch
2 times, most recently
from
bf1acd7 to
817f19f
Compare
| ListOptions: config, | ||
| names: view.GetAllNames(), | ||
|
|
||
| ContainerListOptions: config, |
There was a problem hiding this comment.
Maybe we should un-embed this one, but haven't looked closely yet if that would require a lot of changes, so keeping that for a future exercise
thaJeztah
force-pushed
the
move_container_options
branch
3 times, most recently
from
43efc04 to
f7933ac
Compare
thaJeztah
force-pushed
the
move_container_options
branch
from
f7933ac to
e9ce239
Compare
Move the option-types to the client and in some cases create a copy for the backend. These types are used to construct query- args, and not marshaled to JSON, and can be replaced with functional options in the client. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
move_container_options
branch
from
e9ce239 to
57ce548
Compare
austinvazquez
left a comment
austinvazquez
left a comment
There was a problem hiding this comment.
Changes LGTM.
Not needed here, but just thinking out loud as part of the client module functional options rework I wouldn't mind if we consolidated some of the files down. I noticed before some options where having the _opt.go file just for the type. I liked the conciseness this PR has but maybe even going a step further and putting all container functions into container.go.
| func (cli *Client) ServiceLogs(ctx context.Context, serviceID string, options container.LogsOptions) (io.ReadCloser, error) { | ||
| func (cli *Client) ServiceLogs(ctx context.Context, serviceID string, options ContainerLogsOptions) (io.ReadCloser, error) { |
There was a problem hiding this comment.
+1, separate service log options. Can be follow-up PR though if you want.
Organising the files is always a bit tricky; we need to look what works best. We originally started with a single file for all CLI commands, which became much too large and resulted in merge conflicts between pull requests all the time; later they go split to "per object" (containers, volumes). But some commands (like container) started to grow many subcommands, and with those came utilities for those subcommands, and things got ugly again with merge conflicts, and code ending up in random places. The current organisation somewhat helps with that, and can help guide users to the right place to look; als keeping the tests together (which definitely often was where things got messy, also resulting in duplcate / overlapping tests). Admitted; with the work on functional options, the larger part of code will likely end up in those options, not the command-methods, so possibly what remains will be just "boilerplate" (but maybe in that case the boilerplate can be generated?), let's look at that when we start working on things.
Some of those may have originated from the api, with the expectation for those to be swapped from types generated from swagger, but yeah, with the functional options, it's not unlikely that especially the options will be large, so we need to look what works best. |
|
I'll bring this one in; had a WIP follow-up for the checkpoint-options that I'll open after this one |
thaJeztah
added
impact/changelog
area/go-sdk
impact/go-sdk
3 participants
api/typesdo not relate to the Engine API #50740api/types/container: move container options to client
Move the option-types to the client and in some cases create a
copy for the backend. These types are used to construct query-
args, and not marshaled to JSON, and can be replaced with functional
options in the client.
client: move container options together with their users
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)