scp: add option types #24240
scp: add option types #24240
Conversation
github-actions
bot
added
the
kind/api-change
pkg/api/handlers/libpod/images.go
Outdated
| @@ -713,18 +713,22 @@ func ImageScp(w http.ResponseWriter, r *http.Request) { | |||
|
|
|||
| sourceArg := utils.GetName(r) | |||
|
|
|||
| rep, source, dest, _, err := domainUtils.ExecuteTransfer(sourceArg, query.Destination, []string{}, query.Quiet, ssh.GolangMode) | |||
| opts := entities.ScpExecuteTransferOptions{} | |||
| opts.ParentFlags = []string{} | |||
There was a problem hiding this comment.
Feels more natural to have ExecuteTransfer be tolerant of a nil here so callers don't have to explicitly set this
There was a problem hiding this comment.
I guess it should already tolerate nil, since the only place ParentFlags is really used is here
Which should behave the same if it's nil vs an empty array.
I'll amend to remove the explicit setting you reference though since it should be fine without. Though there are no tests in place to confirm this, the tests for scp in general are pretty sparse.
There was a problem hiding this comment.
The parent flags that are passed to ExecuteTransfer end up getting passed directly to Transfer for local transfers though, so I'll have to verify that would tolerate nil as well.
There was a problem hiding this comment.
While on that subject, would you think that those functions should get options structs as well?
There was a problem hiding this comment.
The signatures don't seem all that bad, but they're certainly not clean either. Wouldn't call it a requirement but wouldn't say no either.
There was a problem hiding this comment.
I just amended to change Transfer's signature because it's public and I could see the need for passing options for one reason or another. I left the other two the same though bc they're private and they could be changed as needed anyways.
Prior to this commit, many scp functions existed without option structs, which would make extending functionality (adding new options) impossible without breaking changes, or without adding redundant wrapper functions. This commit adds in new option types for various scp related functions, and changes those functions' signatures to use the new options. This commit also modifies the `ImageEngine.Scp()` function's interface to use the new opts. The commit also renames the existing `ImageScpOptions` entity type to `ScpTransferImageOptions`. This is because the previous `ImageScpOptions` was inaccurate, as it is not the actual options for `ImageEngine.Scp()`. `ImageEngine.Scp()` should instead receive `ImageScpOptions`. This commit should not change any behavior, however it will break the existing functions' signatures. Signed-off-by: Zachary Hanham <[email protected]>
|
LGTM |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan, zackattackz 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:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
Thanks y'all 😄 |
Prior to this commit, many scp functions existed without option structs, which would make extending functionality (adding new options) impossible without breaking changes, or without adding redundant wrapper functions.
This commit adds in new option types for various scp related functions, and changes those functions' signatures to use the new options.
This commit also modifies the
ImageEngine.Scp()function's interface to use the new opts.The commit also renames the existing
ImageScpOptionsentity type toScpTransferImageOptions. This is because the previousImageScpOptionswas inaccurate, as it is not the actual options forImageEngine.Scp().ImageEngine.Scp()should instead receiveImageScpOptions.This commit should not change any behavior, however it will break the existing functions' signatures.
--
Created this PR to replace #24234 to go ahead and make the breaking changes.
Worth noting: I selected the functions to change pretty arbitrarily. Just based on if I think they have a realistic chance of needing to change in order to add new features. I based it on my experience working on #24211 so it is at least grounded in some way. But I am open to feedback if more or fewer functions should be changed.
Does this PR introduce a user-facing change?