Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: push client functionality and CLI command #147

Merged
merged 19 commits into from Nov 9, 2023

Conversation

anpep
Copy link
Collaborator

@anpep anpep commented Sep 1, 2022

This PR previously contained the implementations of both the push and pull commands and client functionalities. This PR has been refactored to now include only the push relevant code, with the pull functionality present in #148. This allows for the individual PRs to be kept small. Thanks for your feedback!

This is a rather small proof-of-concept implementation of the PushFile()
client API, to explore the sending of files using streams. There are
certain pain points which I would greatly appreciate feedback for, like:

 * The `fileUploader` struct (which I would refactor into a small set of
   functions, if possible)
 * Error handling/logging within the goroutine in charge of uploading
   the files.
 * Testing, taking into account that nearly no tests were written, is
   there a way of testing the multipart body from within the client
   test suite? I didn't find `cs.req` particularly useful...

Other than that, I would like feedback on the approach itself -- I might
be misusing streams here and maybe there's a simpler/safer/faster
method.
Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

I think this is a good start, thanks! I've suggested a significant simplification to the implementation to avoid the goroutine -- let me know what you think.

For Pull, we could us the signature from the spec, like so:

func (*Client) Pull(opts *PullOptions) (*PushResult, error)

type PullOptions struct {
    Path string
}

type PullResult struct {
    Reader io.ReadCloser
}

This is similar to the Python client's pull method, which takes a path and returns a reader.

However, I'm thinking now that for the Go client it would be simpler to implement and to use in the normal case (writing the pulled content to a file) if instead of returning a ReadCloser, it takes a Dest io.Writer in the PullOptions struct. To write to a file, you just pass in the file open for writing as Dest. So it's easy to use, plus the implementation can again be simple, without goroutines.

So I guess I'm advocating for:

func (*Client) Pull(opts *PullOptions) error

type PullOptions struct {
    Path string
    Dest io.Writer
}

client/files.go Outdated Show resolved Hide resolved
client/files.go Outdated Show resolved Hide resolved
client/files.go Outdated Show resolved Hide resolved
client/files.go Outdated Show resolved Hide resolved
client/files.go Outdated Show resolved Hide resolved
client/files_test.go Show resolved Hide resolved
 * Use `Push` instead of `PushFile` for consistency with the Python API.
 * Use an `io.Reader` source instead of a local file path.
 * Ditch `fileUploader` in favor of `io.MultiReader`
 * Complete test coverage
@anpep anpep changed the title Add PoC implementation of PushFile() Implement file push functionality Sep 2, 2022
@anpep anpep changed the title Implement file push functionality Implement push/pull functionality Sep 2, 2022
On older versions of Go, `multipart.Part.FileName()` includes the
directory path information, which doesn't comply with RFC 7578.

In order to accomodate the tests for older Go versions, the base name is
obtained regardless.
@anpep
Copy link
Collaborator Author

anpep commented Sep 6, 2022

I've implemented client.Pull() and the pebble pull command, but there are a lot of things that can be ironed out, in particular:

  • Due to a dependency on http.Response.Body for parsing the multipart body of the response, I was unable to use any of the client.raw()/client.do()/client.doSync() methods and basically had to copy over the logic from client.raw(). Due to the lack of a better mechanism (please correct me if I'm wrong), I propose the Body member from the http.Response object on client.raw() to be exposed in the currently-empty client.ResultInfo struct. Maybe there's a better way to achieve this, feedback welcome!
  • The client.Pull() method returns an struct with an io.ReadCloser associated to the multipart file. However, calling .Close() on this struct will not close the http.Response.Body that is left open. Maybe we can wrap this io.ReadCloser in a struct like this?
    type PullReadCloser struct {
        reader io.ReadCloser
        body io.Closer
    }
    func (p PullReadCloser) Read(b []byte) (n int, err error) {
        return p.reader.Read(b)
    }
    func (p PullReadCloser) Close() error {
        if err := p.reader.Close(); err != nil {
            return err
        }
        if err := p.body.Close(); err != nil {
            return err
        }
    }
  • I still need to write some tests, but I wanted to get this commit a bit early so I could gather feedback before I go on.

 * Implement client.Pull().
 * Implement `pebble pull` command.
 * Patch `pebble push` command to correctly display byte units after transfer.
 * Patch server-side files API to include Content-Length on multipart payload.
   This is required for the pull command to determine transfer progress.
Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Thanks, good work! Add a number of comments. The main thing we need to lock down is the signature of Pull and PullOptions.

internal/daemon/api_files.go Outdated Show resolved Hide resolved
client/files.go Outdated Show resolved Hide resolved
client/files.go Outdated Show resolved Hide resolved
client/files.go Outdated Show resolved Hide resolved
client/files.go Outdated Show resolved Hide resolved
client/files.go Outdated Show resolved Hide resolved
client/files.go Outdated Show resolved Hide resolved
cmd/pebble/cmd_push.go Outdated Show resolved Hide resolved
cmd/pebble/cmd_pull.go Outdated Show resolved Hide resolved
This undoes a breaking change introduced in the previous commit.
 * Pull functionality has been removed from this PR and will be
   introduced in a new one to keep this PR small.
 * Better test coverage for the `pebble push` command
 * Proper handling of quotes in the Content-Disposition header.
@anpep anpep changed the title Implement push/pull functionality Add push client functionality and command Sep 8, 2022
anpep added a commit to anpep/pebble that referenced this pull request Sep 8, 2022
This PR isolates the pull functionality that was removed from the
previous PR (canonical#147) and introduces the following changes:

 * Use a destination `io.Writer` instead of a local file path or
   `io.Reader`.
 * Includes close to perfect coverage of the `pebble pull` command and
   client functionality.

Feedback appreciated!
@anpep anpep requested a review from benhoyt September 8, 2022 18:05
Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Changes look good, thanks for that!

@anpep anpep marked this pull request as ready for review September 13, 2022 07:31
Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

We skimmed through this with Gustavo during our London mini-sprint, and it looks good. He's going to take one more pass before we merge.

cmd/pebble/cmd_push.go Outdated Show resolved Hide resolved
cmd/pebble/cmd_push.go Outdated Show resolved Hide resolved
cmd/pebble/cmd_push.go Outdated Show resolved Hide resolved
 * Use client.Error instead of errorResult.
 * Updated command-line option descriptions.
 * Use compact JSON style.
client/files.go Show resolved Hide resolved
@benhoyt
Copy link
Contributor

benhoyt commented Mar 10, 2023

@anpep Care to bring this branch up to date, and we'll see if we can get it over the goal line before too long?

@jnsgruk jnsgruk requested a review from niemeyer August 1, 2023 15:54
@benhoyt
Copy link
Contributor

benhoyt commented Aug 2, 2023

FWIW, this has been requested a few times, by @sed-i on the Observability team most recently.

Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Hi @anpep, I'd love to bring this up to date and merged soon, as the CLI push and pull commands have been requested by more than one person. Let's merge in the latest master and update to using the new client.Requester().Do() method.

I've also left a few minor comments -- some tweaks, and one real change: to make Permissions an integer os.FileMode rather than a string.

client/files.go Outdated Show resolved Hide resolved
client/files.go Outdated Show resolved Hide resolved
client/files.go Outdated Show resolved Hide resolved
client/files.go Outdated Show resolved Hide resolved
client/files.go Outdated Show resolved Hide resolved
client/files.go Outdated Show resolved Hide resolved
internals/cli/cmd_push.go Outdated Show resolved Hide resolved
internals/cli/cmd_push.go Outdated Show resolved Hide resolved
@benhoyt benhoyt changed the title Add push client functionality and command feat: push client functionality and CLI command Nov 1, 2023
@flotter flotter self-requested a review November 2, 2023 09:15
Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. Just a couple of minor tweaks requested.

client/files.go Outdated Show resolved Hide resolved
internals/cli/cmd_push.go Outdated Show resolved Hide resolved
internals/cli/cmd_push.go Outdated Show resolved Hide resolved
Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the updates. Approving -- I'll aim to merge this in the next couple of days.

client *client.Client

Parents bool `short:"p"`
Mode string `short:"m"`
Copy link
Contributor

Choose a reason for hiding this comment

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

@anpep FYI, I changed these fields names to Parents and Mode so they matched the 'p' and 'm' (before it was confusing because MakeDirs went with 'p' and Permissions went with 'm'). I've done the similar change in cmd_mkdir.go.

I realize this makes the names different from the client field names, but that's okay.

@benhoyt benhoyt merged commit b36325c into canonical:master Nov 9, 2023
11 of 16 checks passed
thp-canonical added a commit to thp-canonical/pebble that referenced this pull request Nov 20, 2023
These features have been implemented now:

* rm (canonical#146)
* push (canonical#147)
* pull (canonical#148)
benhoyt pushed a commit that referenced this pull request Nov 20, 2023
These features have been implemented now. Update README.md to say so:

* rm (#146)
* push (#147)
* pull (#148)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants