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: pull client functionality and CLI command #148

Merged
merged 14 commits into from
Nov 15, 2023
Merged

Conversation

anpep
Copy link
Collaborator

@anpep anpep commented Sep 8, 2022

This PR isolates the pull functionality that was removed from the previous PR (#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!

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!
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_pull.go Outdated Show resolved Hide resolved
cmd/pebble/cmd_pull.go Outdated Show resolved Hide resolved
client/files.go Outdated Show resolved Hide resolved
client/files.go Outdated Show resolved Hide resolved
@benhoyt benhoyt marked this pull request as ready for review September 21, 2022 00:20
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
Copy link
Contributor

@flotter flotter left a comment

Choose a reason for hiding this comment

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

I do not yet have the Go experience give any authoritative approvals. However, I deeply studied both the pull and push code, and compared the expectations of the daemon side, and it looks good for me.

The only observation, which I highlighted on the push PR, is the daemons ability to handle multiple files in a request, and the client side only ever requests a single file, which seems like a sensible starting point.

@anpep
Copy link
Collaborator Author

anpep commented Jan 19, 2023

I do not yet have the Go experience give any authoritative approvals. However, I deeply studied both the pull and push code, and compared the expectations of the daemon side, and it looks good for me.

The only observation, which I highlighted on the push PR, is the daemons ability to handle multiple files in a request, and the client side only ever requests a single file, which seems like a sensible starting point.

Thanks for your feedback on both PRs!

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.

@anpep Looking good -- no further comments. Could you also bring this one up to date, and maybe we can get it merged before too long as well? (Though I realize it'll conflict again with #147 if we merge that first.)

@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.

@flotter flotter self-requested a review November 2, 2023 09:15
@anpep anpep changed the title Add pull client functionality and command feat: pull client functionality and CLI command Nov 6, 2023
@anpep
Copy link
Collaborator Author

anpep commented Nov 7, 2023

I've brought this PR up to date. I'd like to receive feedback on this one, especially on the part where I handle response headers. I decided to write a simple headerToMap() function which converts a http.Header to a map[string]string. Note that the underlying type behind http.Header is map[string][]string (in order to handle multiple headers with the same name), but I wanted to retain the same type we use for RequestOptions:

type RequestOptions struct {
    // ...
    Headers map[string]string
}

type RequestResponse struct {
    // ...
    Headers map[string]string
}

@anpep anpep requested a review from benhoyt November 7, 2023 07:39
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.

Just the Headers field type change (as discussed) and some very nitty things to change.

client/client.go Outdated Show resolved Hide resolved
client/client.go Show resolved Hide resolved
client/client.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 Outdated Show resolved Hide resolved
internals/cli/cmd_help.go Outdated Show resolved Hide resolved
internals/cli/cmd_pull_test.go Outdated Show resolved Hide resolved
@anpep anpep requested a review from benhoyt November 14, 2023 13:19
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.

One more tweak Header -> Headers, and also we need to merge from master and fix conflicts. Thanks!

client/client.go Outdated
@@ -61,6 +61,7 @@ type RequestOptions struct {

type RequestResponse struct {
StatusCode int
Header http.Header
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to nitpick, but I think I'd like this called Headers plural to match the field name in RequestOptions (I've always found it weird that the net/http field name is singular anyway).

internals/cli/cmd_help.go Outdated Show resolved Hide resolved
@anpep
Copy link
Collaborator Author

anpep commented Nov 15, 2023

Sorry about the incomplete changes @benhoyt! I absolutely forgot we already merged push into master (:

@anpep anpep added the Reviewed label Nov 15, 2023
@anpep anpep self-assigned this Nov 15, 2023
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.

Looking good!

@benhoyt benhoyt merged commit cf7aea7 into canonical:master Nov 15, 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.

3 participants