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

Add system tests for bucket notifications. #4021

Merged
merged 6 commits into from
Oct 5, 2017
Merged

Add system tests for bucket notifications. #4021

merged 6 commits into from
Oct 5, 2017

Conversation

tseaver
Copy link
Contributor

@tseaver tseaver commented Sep 21, 2017 •
edited
Loading

N.B.: the tests currently fail with a 403:

____________ TestStorageNotificationCRUD.test_notification_explicit ____________
Traceback (most recent call last):
  File "/home/tseaver/projects/agendaless/Google/src/google-cloud-python/storage/tests/system.py", line 628, in test_notification_explicit
    retry_429(notification.create)()
  File "/home/tseaver/projects/agendaless/Google/src/google-cloud-python/storage/.nox/sys-3-6/lib/python3.6/site-packages/test_utils/retry.py", line 95, in wrapped_function
    return to_wrap(*args, **kwargs)
  File "/home/tseaver/projects/agendaless/Google/src/google-cloud-python/storage/google/cloud/storage/notification.py", line 243, in create
    data=properties,
  File "/home/tseaver/projects/agendaless/Google/src/google-cloud-python/storage/.nox/sys-3-6/lib/python3.6/site-packages/google/cloud/_http.py", line 293, in api_request
    raise exceptions.from_http_response(response)
google.api.core.exceptions.Forbidden: 403 POST https://www.googleapis.com/storage/v1/b/notification-explicit-1506029348265/notificationConfigs: The service account 'precise-truck-742@gs-project-accounts.iam.gserviceaccount.com' does not have permission to publish messages to to the Cloud Pub/Sub topic '//pubsub.googleapis.com/projects/precise-truck-742/topics/notification-1506029282321', or that topic does not exist.
____________ TestStorageNotificationCRUD.test_notification_minimal _____________
Traceback (most recent call last):
  File "/home/tseaver/projects/agendaless/Google/src/google-cloud-python/storage/tests/system.py", line 607, in test_notification_minimal
    retry_429(notification.create)()
  File "/home/tseaver/projects/agendaless/Google/src/google-cloud-python/storage/.nox/sys-3-6/lib/python3.6/site-packages/test_utils/retry.py", line 95, in wrapped_function
    return to_wrap(*args, **kwargs)
  File "/home/tseaver/projects/agendaless/Google/src/google-cloud-python/storage/google/cloud/storage/notification.py", line 243, in create
    data=properties,
  File "/home/tseaver/projects/agendaless/Google/src/google-cloud-python/storage/.nox/sys-3-6/lib/python3.6/site-packages/google/cloud/_http.py", line 293, in api_request
    raise exceptions.from_http_response(response)
google.api.core.exceptions.Forbidden: 403 POST https://www.googleapis.com/storage/v1/b/notification-minimal-1506029351126/notificationConfigs: The service account 'precise-truck-742@gs-project-accounts.iam.gserviceaccount.com' does not have permission to publish messages to to the Cloud Pub/Sub topic '//pubsub.googleapis.com/projects/precise-truck-742/topics/notification-1506029282321', or that topic does not exist.

See #3956 https://github.com/GoogleCloudPlatform/google-cloud-common/issues/231 where I ask for help on that issue.

@tseaver tseaver added api: storage Issues related to the Cloud Storage API. do not merge Indicates a pull request not ready for merge, due to either quality or timing. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels Sep 21, 2017
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 21, 2017
dhermes
dhermes reviewed Sep 21, 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.

No real complaints here

storage/nox.py
@@ -71,10 +71,11 @@ def system_tests(session, python_version):
# virutalenv's dist-packages.
session.install('mock', 'pytest', *LOCAL_DEPS)
session.install('../test_utils/')
session.install('.')
session.install('../pubsub')
session.install('-e', '.')

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

storage/tests/system.py
with Config.CLIENT.batch():
for bucket_name in self.case_buckets_to_delete:
bucket = Config.CLIENT.bucket(bucket_name)
retry_429(bucket.delete)()

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

This comment was marked as spam.

Sign in to view

storage/tests/system.py
self.assertEqual(len(notifications), 1)
self.assertEqual(notifications[0].topic_name, self.TOPIC_NAME)
finally:
notification.delete()

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

storage/tests/system.py Outdated
self.assertIsNotNone(notification.id)
notifications = list(bucket.list_notifications())
self.assertEqual(len(notifications), 1)
self.assertEqual(notifications[0].topic_name, self.TOPIC_NAME)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@tseaver tseaver changed the title WIP: Add system tests for bucket notifications. Add system tests for bucket notifications. Sep 22, 2017
@tseaver tseaver removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Sep 22, 2017
@tseaver
Copy link
Contributor Author

tseaver commented Sep 22, 2017

7d9ee94 updates the notification system tests, adding the IAM grant to the topic required to allow creation of the notification. Those tests are now passing (61c5d0f and da6dc1c fix bugs uncovered by running them).

@tseaver tseaver mentioned this pull request Sep 22, 2017
Closed
8 tasks
@tseaver
Copy link
Contributor Author

tseaver commented Sep 22, 2017 •
edited
Loading

Branch build with new system tests passes the storage system tests. CircleCI failures are unrelated vision flakiness (#4035)

@tseaver tseaver force-pushed the 3956-storage-notifications branch from 9eb9a46 to 352aeed Compare September 25, 2017 15:32
@tseaver
Copy link
Contributor Author

tseaver commented Sep 25, 2017

I rebased the target branch against master after the merge of #4050, and rebased this PR's branch against that branch: here's hoping the "dandruff shampoo" worked.

BrandonY
BrandonY reviewed Sep 27, 2017
View reviewed changes
storage/tests/system.py
policy = self.publisher_client.get_iam_policy(self.topic_path)
binding = policy.bindings.add()
binding.role = 'roles/pubsub.publisher'
binding.members.append(

This comment was marked as spam.

Sign in to view

@tseaver tseaver mentioned this pull request Oct 2, 2017
Closed
@tseaver tseaver merged commit 978eb0f into googleapis:3956-storage-notifications Oct 5, 2017
@tseaver tseaver deleted the 3956-storage-notifications-systest branch October 5, 2017 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BrandonY BrandonY BrandonY left review comments

@dhermes dhermes dhermes left review comments

@lukesneeringer lukesneeringer Awaiting requested review from lukesneeringer

Assignees
No one assigned
Labels
api: storage Issues related to the Cloud Storage API. cla: yes This human has signed the Contributor License Agreement. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tseaver @dhermes @BrandonY @googlebot