Fetch remote organization via action api #121
Conversation
Organizations used to be returned by /api/2/rest/group, this is what the old implementation used to fetch the information to create the remote organization on the local instance of CKAN. With this commit the Action API is used to fetch the same information.
First try to get a remote org from the remote Action API, if this fails try to use the old rest api call, which works on older CKAN versions. Only if both options fail, its currently not possible to get the remote organization.
| org = self._get_group(harvest_object.source.url, remote_org) | ||
| try: | ||
| org = self._get_organization(harvest_object.source.url, remote_org) | ||
| except: |
There was a problem hiding this comment.
@metaodi this is bad, see comment above
|
PS Cheers for submitting this - most helpful |
1. Try whenever possible to catch specific exceptions 2. Raise custom exception where appropriate 3. Fix the exception handling in _get_group and _get_organization
The API call /api/2/rest/package/<id> returns the display name of the group instead of its ID. To properly match the group, munge the name before calling /api/2/rest/group
|
@davidread @amercader okay, I tried to clean this all up. I introduced two now *Error classes, one to wrap the urllib2 exception and one to wrap either the HTTP or the json parse error. I replaced all generic During my tests I also found an issue with the remote_groups, if you want me to create a separate PR for that, just let me know. |
| raise ContentFetchError( | ||
| 'Could not fetch url: %s, HTTP error code: %s' % | ||
| (url, e.code) | ||
| ) |
There was a problem hiding this comment.
I believe this also needs to catch urllib2.URLError, in cases such as where the url has bad syntax, or the socket connection times out.
There was a problem hiding this comment.
Good catch, I'll replace it with urllib2.URLError (which is the super class of urllib2.HTTPError), this should cover the exceptions at this point.
|
Thanks @metaodi - I think that's another really valuable improvement |
HTTPError is a subclass of URLError, so catch URLError is enough. I think the HTTP error code is not as important in this situation, so catching the more generic error seems like the best solution.
|
I didn't know about HTTPError inheriting URLError - cheers! |
|
@davidread a thanks for this hint, replaced it with str() as you suggested. |
Fetch remote organization via action api
|
Thanks @metaodi |
Organizations used to be returned by
/api/2/rest/group, this is what theold implementation used to fetch the information to create the remote
organization on the local instance of CKAN.
With this commit the Action API is used to fetch the same information.
Fixes #120