Storage: ProjectID Regex by chemelnucfin · Pull Request #4543 · googleapis/google-cloud-python

chemelnucfin · 2017-12-07T03:33:44Z

ProjectID description from https://cloud.google.com/resource-manager/reference/rest/v1beta1/projects#Project.FIELDS.project_id

#4447

chemelnucfin · 2017-12-07T04:18:48Z

Hmm. it is locally 100%. Investigating

chemelnucfin · 2017-12-07T05:50:05Z

Strange. Let's see if #4544 fixes it.

Sign in to view


 name = match.group('name')
 project = match.group('project')
+ if project and len(project) > 30:


Sign in to view


+class TestProjectID(unittest.TestCase):
+
+ from google.cloud.storage import notification


Sign in to view

+ PROJECT = notification._PROJECT_PATTERN
+
+ def test_min_length_fail(self):
+ import re


Sign in to view

+
+ def test_leading_digit_fail(self):
+ import re
+ project = re.match(self.PROJECT, '1''a' * 5)


Sign in to view


 _TOPIC_REF_FMT = '//pubsub.googleapis.com/projects/{}/topics/{}'
-_PROJECT_PATTERN = r'(?P<project>[a-z][a-z0-9-]{4,28}[a-z0-9])'
+_PROJECT_PATTERN = r'(?P<project>\A[a-z][a-z0-9-]{4,28}[a-z0-9]\Z)'


Sign in to view

 _TOPIC_NAME_PATTERN = r'(?P<name>[A-Za-z](\w|[-_.~+%])+)'
 _TOPIC_REF_PATTERN = _TOPIC_REF_FMT.format(
- _PROJECT_PATTERN, _TOPIC_NAME_PATTERN)
+ _PROJECT_PATTERN.replace(r'^', '').replace(r'$', ''),


dhermes

@chemelnucfin Let's make sure to get this merged tomorrow?

Sign in to view

 _TOPIC_NAME_PATTERN = r'(?P<name>[A-Za-z](\w|[-_.~+%])+)'
 _TOPIC_REF_PATTERN = _TOPIC_REF_FMT.format(
- _PROJECT_PATTERN, _TOPIC_NAME_PATTERN)
+ _PROJECT_PATTERN.replace(r'\A', '').replace(r'\Z', ''),


Sign in to view

 import mock


+class TestProjectID(unittest.TestCase):


Sign in to view

+ pattern = notification._PROJECT_PATTERN
+ project = re.match(pattern, 'z0123456789\nmalicious', re.M)
+ self.assertIsNone(project)
+


googlebot · 2017-12-12T22:47:06Z

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.

dhermes · 2017-12-12T22:49:45Z

@chemelnucfin I did a small re-factor to push this over the finish line.

Some things worth noting:

Setting self.TOPIC_REF in those test cases will leave lingering shared state, which we avoid as much as we can (at least in unit tests)
Since the number of from_api_repr tests overwhelmed the rest, I feel it is much easier to read by putting it in a standalone helper (note how much shorter each test case became)
We should probably use @pytest.mark.parametrize here, but that's for another day, another PR (I collapsed all of the "success" cases into one)

chemelnucfin · 2017-12-12T23:35:48Z

@dhermes

Hmm. I guess I was just using what was there before. My bad for not paying attention to assigning to self.TOPIC_REF

2, 3. ok. Sometimes I'm hesitant to refactor for fear of breaking things.

So is it ok to merge?

dhermes · 2017-12-12T23:37:46Z

@chemelnucfin Yes go ahead and merge (make sure to squash).

…er/reference/rest/v1beta1/projects#Project.FIELDS.project_id

Made the regex check in `BucketNotification.from_api_repr` into a standalone helper. This way all the tests can be in "one place".

* Storage: ProjectID Regex from https://cloud.google.com/resource-manager/reference/rest/v1beta1/projects#Project.FIELDS.project_id * Re-factor as part of review process. Made the regex check in `BucketNotification.from_api_repr` into a standalone helper. This way all the tests can be in "one place".

chemelnucfin requested a review from lukesneeringer as a code owner December 7, 2017 03:33

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 7, 2017

chemelnucfin force-pushed the issue4447-projectregex branch from 9becd22 to 98c9695Compare December 7, 2017 04:32

chemelnucfin added api: storage Issues related to the Cloud Storage API. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p2 Moderately-important priority. Fix may not be included in next release. labels Dec 7, 2017

dhermes reviewed Dec 7, 2017
View reviewed changes

chemelnucfin force-pushed the issue4447-projectregex branch 3 times, most recently from a5a865b to 563bcc2Compare December 7, 2017 18:17

dhermes reviewed Dec 7, 2017
View reviewed changes

chemelnucfin force-pushed the issue4447-projectregex branch 3 times, most recently from 21ac69c to 1ae80fdCompare December 7, 2017 20:41

dhermes reviewed Dec 8, 2017
View reviewed changes

dhermes reviewed Dec 12, 2017
View reviewed changes

dhermes force-pushed the issue4447-projectregex branch from 40335cf to 89e5c89Compare December 12, 2017 22:47

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 12, 2017

chemelnucfinand others added 2 commits December 12, 2017 18:13

Storage: ProjectID Regex fromhttps://cloud.google.com/resource-manag…
be7b734
…er/reference/rest/v1beta1/projects#Project.FIELDS.project_id

Re-factor as part of review process.
6fc8ded
Made the regex check in `BucketNotification.from_api_repr` into a standalone helper. This way all the tests can be in "one place".

chemelnucfin force-pushed the issue4447-projectregex branch from 89e5c89 to 6fc8dedCompare December 13, 2017 02:13

chemelnucfin merged commit fd1b3fd into googleapis:masterDec 13, 2017

chemelnucfin deleted the issue4447-projectregex branch December 13, 2017 03:50

chemelnucfin mentioned this pull request Dec 13, 2017
Storage: This doesn't match our existing project ID: hello-rightscale002 #4447
Closed

tseaver mentioned this pull request Jan 10, 2018
Prep storage-1.7.0 release. #4729
Merged


		class TestProjectID(unittest.TestCase):

		from google.cloud.storage import notification

Uh oh!

Conversation

chemelnucfin commented Dec 7, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chemelnucfin commented Dec 7, 2017

Uh oh!

chemelnucfin commented Dec 7, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

dhermes left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

googlebot commented Dec 12, 2017

Uh oh!

dhermes commented Dec 12, 2017

Uh oh!

chemelnucfin commented Dec 12, 2017

Uh oh!

dhermes commented Dec 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chemelnucfin commented Dec 7, 2017•
edited
Loading

chemelnucfin commented Dec 7, 2017•
edited
Loading