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

Add LogStream #148

Merged
merged 2 commits into from
Aug 11, 2020
Merged

Add LogStream #148

merged 2 commits into from
Aug 11, 2020

Conversation

edbaunton
Copy link
Collaborator

Add the logstream API, originally pushed upstream by Eric Burnett to
gist.github.com/EricBurnett/a094e0b8474b834d098e86bf20fa202b via
groups.google.com/g/remote-execution-apis/c/LCLsBSgSnU0/m/yTTtqr-cAwAJ

Update to include a copyright notice; set the version number to v1 and update
the package to reflect the location within the remote-apis repo.

I made no material changes to the logstream api other than:

  • to prefix the proto and namespace to be consistent with the others found in this repo, so that it is remote_logstream.proto.
  • Add a copyright notice
  • Setup the namespaces in the proto mirroring what was done for the asset api

Add the logstream API, originally pushed upstream by Eric Burnett to
https://gist.github.com/EricBurnett/a094e0b8474b834d098e86bf20fa202b via
https://groups.google.com/g/remote-execution-apis/c/LCLsBSgSnU0/m/yTTtqr-cAwAJ

Update to include a copyright notice; set the version number to v1 and update
the package to reflect the location within the remote-apis repo.

Signed-off-by: Ed Baunton <[email protected]>
@googlebot googlebot added the cla: yes Pull requests whose authors are covered by a CLA with Google. label Jul 14, 2020
Copy link
Collaborator

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

hooks/pre-commit Show resolved Hide resolved
Copy link
Collaborator

@EricBurnett EricBurnett 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 doing this Ed!

Two small corrections I think we should make to cases, but otherwise all looks good to me.

build/bazel/remote/logstream/v1/remote_logstream.proto Outdated Show resolved Hide resolved
// A handle to a log (an ordered sequence of bytes).
message LogStream {
// Structured name of the resource in the format:
// {parent=**}/logStreams/{logstream_id}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's correct this while we're here: logStreams -> logstreams. (Resources are usually lowercase, and this is already wrong with having a lowercase l and uppercase S)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Couple more: lines 114, 120, and 121 also thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah good catch! Should be fixed now.

@edbaunton edbaunton changed the title Add Logstream Add LogStream Jul 16, 2020
@edbaunton edbaunton requested a review from EricBurnett August 4, 2020 19:46
@edbaunton edbaunton dismissed EricBurnett’s stale review August 4, 2020 19:46

Addressed comments.

Copy link
Collaborator

@EricBurnett EricBurnett 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 capitalization nit on the few remaining lines (see comment on previous review), but nothing blocking that I see. Thanks Ed!

Add a BUILD file and build the remote_logstream.pb.go component. Add that to
the pre-commit script.

Update remote_logstream.proto to include packaging information for the various
programming languages it can be used from, copying the style from asset.

Lowercase the resource name from "logStreams" to "logstreams" for consistency.

Signed-off-by: Ed Baunton <[email protected]>
@sstriker sstriker merged commit 3f78665 into bazelbuild:master Aug 11, 2020
santigl pushed a commit to santigl/remote-apis that referenced this pull request Aug 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Pull requests whose authors are covered by a CLA with Google.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants