Merge lp:~milo/linaro-patchmetrics/bug1013577 into lp:linaro-patchmetrics
- bug1013577
- Merge into trunk
Status: | Merged | ||||
---|---|---|---|---|---|
Merged at revision: | 365 | ||||
Proposed branch: | lp:~milo/linaro-patchmetrics/bug1013577 | ||||
Merge into: | lp:linaro-patchmetrics | ||||
Diff against target: |
219 lines (+79/-31) 4 files modified
apps/patchmetrics/bin/sync-gerrit-changes.py (+25/-18) apps/patchmetrics/gerrit.py (+18/-9) apps/patchmetrics/gerrit_values.py (+12/-1) apps/patchmetrics/tests/test_gerrit.py (+24/-3) |
||||
To merge this branch: | bzr merge lp:~milo/linaro-patchmetrics/bug1013577 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Deepti B. Kalakeri (community) | Approve | ||
Review via email: mp+112290@code.launchpad.net |
Commit message
Description of the change
Hello,
requesting a new merge: we are still getting the error from gerrit about user not found, but this should be handled correctly. I fixed the error string we get back to be exactly as the one we receive, fixed also the query parameters to use to start a new query from the last value. I renamed one module to use a more generic name in order to store also other values than JSON keys.
- 370. By Milo Casagrande
-
Remove white chars from email, just in case.
- 371. By Milo Casagrande
-
Force string on the query, and remove white spaces.
- 372. By Milo Casagrande
-
Fixed param name on docstring.
Deepti B. Kalakeri (deeptik) wrote : | # |
- 373. By Milo Casagrande
-
Fixed query quoting.
Deepti B. Kalakeri (deeptik) wrote : | # |
=== modified file 'apps/patchmetr
--- apps/patchmetri
+++ apps/patchmetri
@@ -102,10 +102,11 @@
def test_create_
"""Tests the creation of a valid request URL."""
params = {'format': 'JSON', 'n': 100}
- query_params = {'owner': '<email address hidden>', 'sortkey_after': 'abcd'}
+ query_params = {'owner': '<email address hidden>',
+ 'resume_sortkey': 'abcd'}
- "n=100&
+ "n=100&
Should this be fixed to the address the <email address hidden> ?
or have one more test for it ?
- 374. By Milo Casagrande
-
Fixed tests.
Milo Casagrande (milo) wrote : | # |
Thanks Deepti,
fixed those as well, and added a new one.
Deepti B. Kalakeri (deeptik) wrote : | # |
112 query_params = {'owner': '<email address hidden>', 'sortkey_after': 'abcd'}
113 expected_result = (ROOT_REST_URL + CHANGES_QUERY_PATH +
114 QUERY_STRING_START + "n=100&
115 "person%
Should not this be resume_key instead of sortkery_after ?
- 375. By Milo Casagrande
-
Fixed wrong query parameter.
Milo Casagrande (milo) wrote : | # |
> Should not this be resume_key instead of sortkery_after ?
Yep, it should be "resume_sortkey", now is fixed.
Thanks again.
Deepti B. Kalakeri (deeptik) wrote : | # |
looks good +1.
Preview Diff
1 | === modified file 'apps/patchmetrics/bin/sync-gerrit-changes.py' | |||
2 | --- apps/patchmetrics/bin/sync-gerrit-changes.py 2012-06-25 09:12:00 +0000 | |||
3 | +++ apps/patchmetrics/bin/sync-gerrit-changes.py 2012-06-27 12:19:18 +0000 | |||
4 | @@ -10,7 +10,7 @@ | |||
5 | 10 | create_or_update_patches_from_aosp_changes, | 10 | create_or_update_patches_from_aosp_changes, |
6 | 11 | get_aosp_changes_from, | 11 | get_aosp_changes_from, |
7 | 12 | ) | 12 | ) |
9 | 13 | from patchmetrics.gerrit_json import ( | 13 | from patchmetrics.gerrit_values import ( |
10 | 14 | GERRIT_MORE_CHANGES, | 14 | GERRIT_MORE_CHANGES, |
11 | 15 | GERRIT_SORT_KEY, | 15 | GERRIT_SORT_KEY, |
12 | 16 | ) | 16 | ) |
13 | @@ -18,30 +18,37 @@ | |||
14 | 18 | 18 | ||
15 | 19 | linaro_engineers = Person.objects.filter( | 19 | linaro_engineers = Person.objects.filter( |
16 | 20 | user__id__in=TeamMembership.objects.values('member').query).distinct() | 20 | user__id__in=TeamMembership.objects.values('member').query).distinct() |
17 | 21 | |||
18 | 21 | for person in linaro_engineers: | 22 | for person in linaro_engineers: |
20 | 22 | start_after = None | 23 | resume_key = None |
21 | 24 | # We are just being a little bit paranoid here. | ||
22 | 25 | user_email = person.email.strip() | ||
23 | 23 | while True: | 26 | while True: |
24 | 24 | try: | 27 | try: |
27 | 25 | results = get_aosp_changes_from(person.email, | 28 | results = get_aosp_changes_from(user_email, resume_key=resume_key) |
26 | 26 | start_after=start_after) | ||
28 | 27 | except GerritError, e: | 29 | except GerritError, e: |
30 | 28 | if '%s not found' % person.email in str(e): | 30 | if ('User %s not found' % user_email) in str(e): |
31 | 29 | # Gerrit returns an error when you search for changes owned by | 31 | # Gerrit returns an error when you search for changes owned by |
32 | 30 | # an email address that's not registered, but in this case we | 32 | # an email address that's not registered, but in this case we |
33 | 31 | # just skip to the next email. | 33 | # just skip to the next email. |
34 | 32 | break | 34 | break |
35 | 33 | raise | 35 | raise |
47 | 34 | # If we get all the changes in a single batch, the GERRIT_MORE_CHANGES | 36 | |
48 | 35 | # key will not be in the JSON output, so we set it to False. | 37 | if results: |
49 | 36 | more_changes = results[-1].get(GERRIT_MORE_CHANGES) or False | 38 | # If we get all the changes in a single batch, the |
50 | 37 | created_or_updated_patches = ( | 39 | # GERRIT_MORE_CHANGES key will not be in the JSON output, so we |
51 | 38 | create_or_update_patches_from_aosp_changes(results, person)) | 40 | # set it to False. |
52 | 39 | 41 | more_changes = results[-1].get(GERRIT_MORE_CHANGES) or False | |
53 | 40 | # If there are less created/updated changes than the number of changes | 42 | created_or_updated_patches = ( |
54 | 41 | # returned by Gerrit we know we can stop as they're sorted most | 43 | create_or_update_patches_from_aosp_changes(results, person)) |
55 | 42 | # recently updated first. | 44 | |
56 | 43 | if len(results) != len(created_or_updated_patches) or \ | 45 | # If there are less created/updated changes than the number of |
57 | 44 | (not more_changes): | 46 | # changes returned by Gerrit we know we can stop as they're sorted |
58 | 47 | # most recently updated first. | ||
59 | 48 | if len(results) != len(created_or_updated_patches) or \ | ||
60 | 49 | (not more_changes): | ||
61 | 50 | break | ||
62 | 51 | |||
63 | 52 | resume_key = results[-1].get(GERRIT_SORT_KEY) | ||
64 | 53 | else: | ||
65 | 45 | break | 54 | break |
66 | 46 | |||
67 | 47 | start_after = results[-1].get(GERRIT_SORT_KEY) | ||
68 | 48 | 55 | ||
69 | === modified file 'apps/patchmetrics/gerrit.py' | |||
70 | --- apps/patchmetrics/gerrit.py 2012-06-25 12:50:28 +0000 | |||
71 | +++ apps/patchmetrics/gerrit.py 2012-06-27 12:19:18 +0000 | |||
72 | @@ -1,17 +1,22 @@ | |||
73 | 1 | from datetime import datetime | 1 | from datetime import datetime |
74 | 2 | import email | 2 | import email |
75 | 3 | import urllib | ||
76 | 3 | import urllib2 | 4 | import urllib2 |
77 | 4 | from urllib import urlencode | ||
78 | 5 | 5 | ||
79 | 6 | import json | 6 | import json |
80 | 7 | 7 | ||
81 | 8 | from patchwork.models import Patch, Project, State | 8 | from patchwork.models import Patch, Project, State |
82 | 9 | from patchmetrics.models import GerritChange | 9 | from patchmetrics.models import GerritChange |
84 | 10 | from patchmetrics.gerrit_json import ( | 10 | from patchmetrics.gerrit_values import ( |
85 | 11 | GERRIT_CREATED_KEY, | 11 | GERRIT_CREATED_KEY, |
86 | 12 | GERRIT_DEFAULT_JSON, | ||
87 | 12 | GERRIT_ID_KEY, | 13 | GERRIT_ID_KEY, |
88 | 13 | GERRIT_NUMBER_KEY, | 14 | GERRIT_NUMBER_KEY, |
89 | 14 | GERRIT_PROJECT_KEY, | 15 | GERRIT_PROJECT_KEY, |
90 | 16 | GERRIT_QUERY_FORMAT, | ||
91 | 17 | GERRIT_QUERY_OWNER, | ||
92 | 18 | GERRIT_QUERY_PG_SIZE, | ||
93 | 19 | GERRIT_QUERY_RESUME_SORTKEY, | ||
94 | 15 | GERRIT_SANITIZABLE_VALUE, | 20 | GERRIT_SANITIZABLE_VALUE, |
95 | 16 | GERRIT_STATUS_KEY, | 21 | GERRIT_STATUS_KEY, |
96 | 17 | GERRIT_SUBJECT_KEY, | 22 | GERRIT_SUBJECT_KEY, |
97 | @@ -40,12 +45,12 @@ | |||
98 | 40 | """The gerrit server returned an error in response to a query.""" | 45 | """The gerrit server returned an error in response to a query.""" |
99 | 41 | 46 | ||
100 | 42 | 47 | ||
103 | 43 | def get_aosp_changes_from(email, start_after=None, page_size=25, | 48 | def get_aosp_changes_from(email, resume_key=None, page_size=25, |
104 | 44 | return_format='JSON_COMPACT'): | 49 | return_format=GERRIT_DEFAULT_JSON): |
105 | 45 | """Get the list of changes authored by the given email address. | 50 | """Get the list of changes authored by the given email address. |
106 | 46 | 51 | ||
107 | 47 | :param email: The email of the user to get the changes of. | 52 | :param email: The email of the user to get the changes of. |
109 | 48 | :param start_after: Only include changes whose 'sortkey_after' is lower | 53 | :param resume_key: Only include changes whose 'sortkey_after' is lower |
110 | 49 | than this. Use None if you want to start from the | 54 | than this. Use None if you want to start from the |
111 | 50 | beginning. | 55 | beginning. |
112 | 51 | :param page_size: Number of items to include in the list, defaults to 25. | 56 | :param page_size: Number of items to include in the list, defaults to 25. |
113 | @@ -53,8 +58,10 @@ | |||
114 | 53 | 'JSON_COMPACT' for JSON format without spaces, also | 58 | 'JSON_COMPACT' for JSON format without spaces, also |
115 | 54 | possible to specify 'JSON'. | 59 | possible to specify 'JSON'. |
116 | 55 | """ | 60 | """ |
119 | 56 | params = {'format': return_format, 'n': page_size} | 61 | params = {GERRIT_QUERY_FORMAT: return_format, |
120 | 57 | query_params = {'owner': email, 'sortkey_after': start_after} | 62 | GERRIT_QUERY_PG_SIZE: page_size} |
121 | 63 | query_params = {GERRIT_QUERY_OWNER: email, | ||
122 | 64 | GERRIT_QUERY_RESUME_SORTKEY: resume_key} | ||
123 | 58 | return rest_api_request(ROOT_REST_URL, CHANGES_QUERY_PATH, params, | 65 | return rest_api_request(ROOT_REST_URL, CHANGES_QUERY_PATH, params, |
124 | 59 | query_params) | 66 | query_params) |
125 | 60 | 67 | ||
126 | @@ -88,7 +95,7 @@ | |||
127 | 88 | path += '/' | 95 | path += '/' |
128 | 89 | request_url = url + path + QUERY_STRING_START | 96 | request_url = url + path + QUERY_STRING_START |
129 | 90 | 97 | ||
131 | 91 | request_url += urlencode(params) | 98 | request_url += urllib.urlencode(params) |
132 | 92 | request_url += create_gerrit_rest_query(query_params) | 99 | request_url += create_gerrit_rest_query(query_params) |
133 | 93 | 100 | ||
134 | 94 | return request_url | 101 | return request_url |
135 | @@ -113,7 +120,9 @@ | |||
136 | 113 | for key, value in query_params.iteritems(): | 120 | for key, value in query_params.iteritems(): |
137 | 114 | # We cannot have None values here, we just skip them. | 121 | # We cannot have None values here, we just skip them. |
138 | 115 | if value is not None: | 122 | if value is not None: |
140 | 116 | params_list.append(":".join((key, value))) | 123 | # We need strings here, and we remove white spaces. |
141 | 124 | params_list.append(":".join((key, | ||
142 | 125 | urllib.quote(str(value).strip())))) | ||
143 | 117 | # The resulting query string, without separators or prefixes, should be in | 126 | # The resulting query string, without separators or prefixes, should be in |
144 | 118 | # the form of: | 127 | # the form of: |
145 | 119 | # key:value+key:value | 128 | # key:value+key:value |
146 | 120 | 129 | ||
147 | === renamed file 'apps/patchmetrics/gerrit_json.py' => 'apps/patchmetrics/gerrit_values.py' | |||
148 | --- apps/patchmetrics/gerrit_json.py 2012-06-22 14:33:32 +0000 | |||
149 | +++ apps/patchmetrics/gerrit_values.py 2012-06-27 12:19:18 +0000 | |||
150 | @@ -1,4 +1,4 @@ | |||
152 | 1 | # File with all the gerrit JSON valid keys. | 1 | # File with the gerrit JSON valid keys and other gerrit values. |
153 | 2 | # For more information about the values defined here, look: | 2 | # For more information about the values defined here, look: |
154 | 3 | # https://gerrit-review.googlesource.com/Documentation/json.html | 3 | # https://gerrit-review.googlesource.com/Documentation/json.html |
155 | 4 | # Also, on bug https://bugs.launchpad.net/linaro-patchmetrics/+bug/1013577, | 4 | # Also, on bug https://bugs.launchpad.net/linaro-patchmetrics/+bug/1013577, |
156 | @@ -14,7 +14,18 @@ | |||
157 | 14 | GERRIT_STATUS_KEY = 'status' | 14 | GERRIT_STATUS_KEY = 'status' |
158 | 15 | GERRIT_PROJECT_KEY = 'project' | 15 | GERRIT_PROJECT_KEY = 'project' |
159 | 16 | GERRIT_SUBJECT_KEY = 'subject' | 16 | GERRIT_SUBJECT_KEY = 'subject' |
160 | 17 | |||
161 | 17 | # Gerrit JSON output starts with a magic prefix, making it not valid JSON. | 18 | # Gerrit JSON output starts with a magic prefix, making it not valid JSON. |
162 | 18 | # For more info: | 19 | # For more info: |
163 | 19 | # https://gerrit-review.googlesource.com/Documentation/rest-api.html | 20 | # https://gerrit-review.googlesource.com/Documentation/rest-api.html |
164 | 20 | GERRIT_SANITIZABLE_VALUE = ")]}'" | 21 | GERRIT_SANITIZABLE_VALUE = ")]}'" |
165 | 22 | |||
166 | 23 | # The possible values for the output format of the Gerrit query. | ||
167 | 24 | GERRIT_DEFAULT_JSON = 'JSON_COMPACT' | ||
168 | 25 | GERRIT_NORMAL_JSON = 'JSON' | ||
169 | 26 | |||
170 | 27 | # Gerrit query parameters | ||
171 | 28 | GERRIT_QUERY_OWNER = 'owner' | ||
172 | 29 | GERRIT_QUERY_FORMAT = 'format' | ||
173 | 30 | GERRIT_QUERY_PG_SIZE = 'n' | ||
174 | 31 | GERRIT_QUERY_RESUME_SORTKEY = 'resume_sortkey' | ||
175 | 21 | 32 | ||
176 | === modified file 'apps/patchmetrics/tests/test_gerrit.py' | |||
177 | --- apps/patchmetrics/tests/test_gerrit.py 2012-06-22 14:33:32 +0000 | |||
178 | +++ apps/patchmetrics/tests/test_gerrit.py 2012-06-27 12:19:18 +0000 | |||
179 | @@ -3,6 +3,13 @@ | |||
180 | 3 | 3 | ||
181 | 4 | from django.test import TestCase | 4 | from django.test import TestCase |
182 | 5 | 5 | ||
183 | 6 | from patchmetrics.gerrit_values import ( | ||
184 | 7 | GERRIT_QUERY_FORMAT, | ||
185 | 8 | GERRIT_QUERY_OWNER, | ||
186 | 9 | GERRIT_QUERY_PG_SIZE, | ||
187 | 10 | GERRIT_QUERY_RESUME_SORTKEY, | ||
188 | 11 | ) | ||
189 | 12 | |||
190 | 6 | from patchmetrics.gerrit import ( | 13 | from patchmetrics.gerrit import ( |
191 | 7 | CHANGES_QUERY_PATH, | 14 | CHANGES_QUERY_PATH, |
192 | 8 | QUERY_STRING_START, | 15 | QUERY_STRING_START, |
193 | @@ -103,9 +110,23 @@ | |||
194 | 103 | """Tests the creation of a valid request URL.""" | 110 | """Tests the creation of a valid request URL.""" |
195 | 104 | params = {'format': 'JSON', 'n': 100} | 111 | params = {'format': 'JSON', 'n': 100} |
196 | 105 | query_params = {'owner': 'person@example.org', 'sortkey_after': 'abcd'} | 112 | query_params = {'owner': 'person@example.org', 'sortkey_after': 'abcd'} |
200 | 106 | expected_result = ROOT_REST_URL + CHANGES_QUERY_PATH + \ | 113 | expected_result = (ROOT_REST_URL + CHANGES_QUERY_PATH + |
201 | 107 | QUERY_STRING_START + \ | 114 | QUERY_STRING_START + "n=100&format=JSON&q=owner:" |
202 | 108 | "n=100&format=JSON&q=owner:person@example.org+sortkey_after:abcd" | 115 | "person%40example.org+resume_sortkey:abcd") |
203 | 116 | self.assertEquals(expected_result, | ||
204 | 117 | create_request_query_url(ROOT_REST_URL, | ||
205 | 118 | CHANGES_QUERY_PATH, | ||
206 | 119 | params, query_params)) | ||
207 | 120 | |||
208 | 121 | def test_create_request_query_url_quoted(self): | ||
209 | 122 | """Tests the creation of a valid request URL with a complex email.""" | ||
210 | 123 | params = {GERRIT_QUERY_FORMAT: 'JSON', GERRIT_QUERY_PG_SIZE: 100} | ||
211 | 124 | query_params = {GERRIT_QUERY_OWNER: 'name+surname.person@example.org', | ||
212 | 125 | GERRIT_QUERY_RESUME_SORTKEY: 'abcd'} | ||
213 | 126 | expected_result = (ROOT_REST_URL + CHANGES_QUERY_PATH + | ||
214 | 127 | QUERY_STRING_START + "n=100&format=JSON&q=owner:" | ||
215 | 128 | "name%2Bsurname.person%40example.org+" | ||
216 | 129 | "resume_sortkey:abcd") | ||
217 | 109 | self.assertEquals(expected_result, | 130 | self.assertEquals(expected_result, |
218 | 110 | create_request_query_url(ROOT_REST_URL, | 131 | create_request_query_url(ROOT_REST_URL, |
219 | 111 | CHANGES_QUERY_PATH, | 132 | CHANGES_QUERY_PATH, |
As discussed on IRC, somehow the email formation/query seems to be incorrect in the existing code even before the changes done here. I will wait for you to push the changes for the query format and will then proceed with the review.
Thanks!!!
Deepti.