-
Notifications
You must be signed in to change notification settings - Fork 57
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(client): add Go client for notices #297
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. A couple of questions for the purpose of learning from this :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. Looks good, with just high-level tuning.
} | ||
|
||
// Notice fetches a single notice by ID. | ||
func (client *Client) Notice(id string) (*Notice, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commenting here to avoid mixing with the unrelated thread below.
Taking a raw string from the outside world and blindly appending it to the GET path with zero validation is anxiety inducing. This is the kind of thing that can be exploited in an application we have no visibility over and become an actual hack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see what you're saying, but I'm not so sure. Go will safely and properly percent-escape any disallowed characters, and the server will unescape (and will not find the notice).
However, I've added a simple client-side validation -- that's slightly more permissive than the integers we produce now for future-proofing -- see what you think.
If you think this is a good approach, I can create a PR to do something similar in client/changes.go
, where we do the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've opened #318 to track that.
To make it compile, also update WaitNotices to use Requester.Do Conflicts: client/client.go internals/daemon/api_notices.go internals/daemon/api_notices_test.go
This is a backport from pebble: canonical/pebble#297 Signed-off-by: Zeyad Gouda <[email protected]>
This is a backport from pebble: canonical/pebble#297 Signed-off-by: Zeyad Gouda <[email protected]>
This is a backport from pebble: canonical/pebble#297 Signed-off-by: Zeyad Gouda <[email protected]>
This is a backport from pebble: canonical/pebble#297 Signed-off-by: Zeyad Gouda <[email protected]>
This adds the Go client for Pebble Notices (see spec JU048).
Here is a summary of the new
Client
methods: