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.

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

Fixing a bug related to custom user for harvesting jobs #226

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

memaldi
Copy link

@memaldi memaldi commented Mar 18, 2016

In my honest opinion, there is a bug in base.py that avoids using custom user identified by the property "user" in configuration JSON.

@amercader
Copy link
Member

@memaldi I'm not sure what you are trying to fix is a bug.
The original behaviour was to look for the user name is a global CKAN configuration setting (config['ckanext.harvest.user_name']), ie one you set on your ini file. Your patch looks for a different key (user) on the own harvest source configuration object (the one you enter in the harvest form).

I can't see a use case for wanting to use a different internal user on different harvest sources so I'm not sure if that was your original intention either.

@memaldi
Copy link
Author

memaldi commented Mar 21, 2016

Taken from the documentation:

The CKAN harvesters support a number of configuration options to control their behaviour. Those need to be defined as a JSON object in the configuration form field. The currently supported configuration options are:
[...]
user: User who will run the harvesting process. Please note that this user needs to have permission for creating packages, and if default groups were defined, the user must have permission to assign packages to these groups.
[...]

It seems that the implementation of the harvester is different from the stated at the documentation. I fixed this because in the project in which I'm working we need that datasets from different sources to be created by different users, because an issue related to the integration of an external component independent to CKAN. And as the documentation shows, it seemed to be possible, but it was not working.

Anyway, if the actual logic of the harvester is the correct one, maybe the bug is in the documentation ;-)

Regards,

@amercader
Copy link
Member

You are absolutely correct @memaldi, sorry, I hadn't looked at the source config options in a while.

Your patch makes sense now, but there's no need to remove the config['ckanext.harvest.user_name'] logic as it serves a different use case.

Also you don't need to call user_show again as we checked it existed when validating the configuration:

https://github.com/ckan/ckanext-harvest/blob/master/ckanext/harvest/harvesters/ckanharvester.py#L137:L144

Other than that I'm happy to merge it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants