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.

Sign up for GitHub

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

Jump to bottom

Logging: Unique Writer Identity #4595

Merged
merged 4 commits into from
Dec 20, 2017
Merged

Logging: Unique Writer Identity #4595

merged 4 commits into from
Dec 20, 2017

Conversation

chemelnucfin
Copy link
Contributor

@chemelnucfin chemelnucfin commented Dec 15, 2017 •
edited
Loading

#4578 Not sure if this is right, but an attempt.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 15, 2017
@chemelnucfin chemelnucfin force-pushed the logging_unique_writer_identity branch 2 times, most recently from 231df62 to 39e4567 Compare December 15, 2017 16:42
duggelz
duggelz reviewed Dec 15, 2017
View reviewed changes
logging/google/cloud/logging/_gax.py
@@ -211,13 +212,22 @@ def sink_create(self, project, sink_name, filter_, destination):
:type destination: str
:param destination: destination URI for the entries exported by
the sink.

:type unique_writer_identity: bool

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

dhermes
dhermes reviewed Dec 15, 2017
View reviewed changes
Copy link
Contributor

@dhermes dhermes left a comment

Choose a reason for hiding this comment

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

Looks mostly good, I just have nitpicks (and one relevant docs change)

logging/google/cloud/logging/_gax.py Outdated
"""
options = None
parent = 'projects/%s' % (project,)
sink_pb = LogSink(name=sink_name, filter=filter_,
destination=destination)
uwi = unique_writer_identity

This comment was marked as spam.

Sign in to view

logging/google/cloud/logging/_gax.py Outdated
self._gax_api.create_sink(parent,
sink_pb,
unique_writer_identity=uwi,
options=options)

This comment was marked as spam.

Sign in to view

logging/google/cloud/logging/sink.py Outdated
@@ -40,11 +40,13 @@ class Sink(object):
:param client: A client which holds credentials and project configuration
for the sink (which requires a project).
"""
def __init__(self, name, filter_=None, destination=None, client=None):
def __init__(self, name, filter_=None, destination=None, client=None,
unique_writer_identity=False):

This comment was marked as spam.

Sign in to view

logging/google/cloud/logging/sink.py Outdated
@@ -116,7 +118,9 @@ def create(self, client=None):
"""
client = self._require_client(client)
client.sinks_api.sink_create(
self.project, self.name, self.filter_, self.destination)
self.project, self.name, self.filter_, self.destination,
self._unique_writer_identity

This comment was marked as spam.

Sign in to view

logging/tests/unit/test__gax.py Outdated
@@ -40,6 +40,7 @@ class _Base(object):
PROJECT = 'PROJECT'
PROJECT_PATH = 'projects/%s' % (PROJECT,)
FILTER = 'logName:syslog AND severity>=ERROR'
UNIQUE_WRITER_IDENTITY = True

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

logging/tests/unit/test__gax.py
@@ -737,7 +738,7 @@ def test_sink_create_ok(self):
api.sink_create(
self.PROJECT, self.SINK_NAME, self.FILTER, self.DESTINATION_URI)

parent, sink, options = (
parent, sink, options, unique_writer_identity = (

This comment was marked as spam.

Sign in to view

logging/tests/unit/test__gax.py Outdated
api = self._make_one(gax_api, None)
api.sink_create(
self.PROJECT, self.SINK_NAME, self.FILTER, self.DESTINATION_URI,
self.UNIQUE_WRITER_IDENTITY

This comment was marked as spam.

Sign in to view

logging/tests/unit/test__gax.py Outdated
self.assertEqual(sink.name, self.SINK_NAME)
self.assertEqual(sink.filter, self.FILTER)
self.assertEqual(sink.destination, self.DESTINATION_URI)
self.assertEqual(unique_writer_identity, self.UNIQUE_WRITER_IDENTITY)

This comment was marked as spam.

Sign in to view

logging/tests/unit/test__http.py Outdated
api = self._make_one(client)

api.sink_create(
self.PROJECT, self.SINK_NAME, self.FILTER, self.DESTINATION_URI, True)

This comment was marked as spam.

Sign in to view

logging/tests/unit/test__http.py Outdated
path = '/projects/%s/sinks' % (self.PROJECT,)
self.assertEqual(conn._called_with['path'], path)
self.assertEqual(conn._called_with['data'], SENT)
self.assertEqual(conn._called_with['query_params'], {'uniqueWriterIdentity': True})

This comment was marked as spam.

Sign in to view

@chemelnucfin chemelnucfin added the api: logging Issues related to the Cloud Logging API. label Dec 19, 2017
@chemelnucfin chemelnucfin force-pushed the logging_unique_writer_identity branch 2 times, most recently from d4bcf59 to 9063794 Compare December 20, 2017 02:04
chemelnucfin and others added 4 commits December 20, 2017 10:26
@chemelnucfin @dhermes
Logging: Unique Writer Identity
2c5464f
@chemelnucfin @dhermes
review changes
81d430a
@chemelnucfin @dhermes
Logging: use alias variable because lint complains about line continu…
1b63395 
…ation
@dhermes
Hygiene updates.
105d327 
- Dropping a `uwi` alias for a long variable name
- Making line continuations a bit more "uniform" (e.g. a single
  value per line)
- Making sure to always pass a keyword argument as a keyword
- Dropping a long class scalar that is just an alias for the `True`
  literal
@dhermes dhermes force-pushed the logging_unique_writer_identity branch from 9063794 to 105d327 Compare December 20, 2017 18:36
@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@googlebot googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Dec 20, 2017
dhermes
dhermes approved these changes Dec 20, 2017
View reviewed changes
Copy link
Contributor

@dhermes dhermes left a comment

Choose a reason for hiding this comment

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

LGTM

@dhermes dhermes merged commit 465a888 into googleapis:master Dec 20, 2017
@dhermes dhermes mentioned this pull request Dec 20, 2017
Closed
@chemelnucfin chemelnucfin added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Dec 20, 2017
@chemelnucfin chemelnucfin deleted the logging_unique_writer_identity branch December 25, 2017 03:32
@tseaver tseaver mentioned this pull request Jan 5, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@duggelz duggelz duggelz left review comments

@dhermes dhermes dhermes approved these changes

@lukesneeringer lukesneeringer Awaiting requested review from lukesneeringer

Assignees
No one assigned
Labels
api: logging Issues related to the Cloud Logging API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@chemelnucfin @googlebot @dhermes @duggelz