[Draft] Proof of concept for xloader site url #234
Conversation
|
CKAN 2.11 build is broken by ckan/ckan-docker-base#86 and needs to run the test container as |
| default: | ||
| description: | | ||
| Provide an alternate site URL for the xloader_submit action. | ||
| This is useful, for example, when the site is running within a docker network. |
There was a problem hiding this comment.
| This is useful, for example, when the site is running within a docker network. | |
| This is useful, for example, when the site is running within a docker network | |
| or where the job runner can't access ckan using its normal public site URL. |
| @@ -2,6 +2,12 @@ version: 1 | |||
| groups: | |||
| - annotation: ckanext-xloader settings | |||
| options: | |||
| - key: ckanext.xloader.site_url | |||
| default: | |||
There was a problem hiding this comment.
I think we don't need an empty string default, it's normal for optional settings to be not present if not provided
| default: |
There was a problem hiding this comment.
instead of the toolkit.config.get("ckanext.xloader.site_url") or toolkit.config.get("ckan.site_url") logic below we should be able to use just toolkit.config.get("ckanext.xloader.site_url") along with:
validators: configured_default("ckan.site_url",None)
There was a problem hiding this comment.
@wardi ok I see. so that default is just set in the config yaml, and then, any code can assume that fallback default for the ckanext.xloader.site_url setting?
There was a problem hiding this comment.
yeah, this validator will set ckanext.xloader.site_url to the same as ckan.site_url when it's not given. Another one of @smotornyuk 's clever ideas.
| site_url_configs = ("ckan.site_url", "ckanext.xloader.site_url") | ||
| if not any(site_url_configs): | ||
| raise Exception( | ||
| f"One of config options {site_url_configs} must be set to use ckanext-xloader." | ||
| ) |
There was a problem hiding this comment.
ckan will refuse to start if a site_url isn't provided, so this code would never get executed
There was a problem hiding this comment.
@wardi here, I'm just extending the existing check for that. I prefer not to remove existing behavior, but just to provide the minimal new behavior required for the PR
https://github.com/ckan/ckanext-xloader/blob/master/ckanext/xloader/plugin.py#L64
There was a problem hiding this comment.
If you did not cleanup the error message, this would not have been commented upon ;)
I'm fine leaving this in as its a belts and braces approach, better to fail early (if it does occur which is very very remote)
| "result_url": callback_url, | ||
| "metadata": { | ||
| "ignore_hash": True, | ||
| "ckan_url": toolkit.config.get("ckanext.xloader.site_url") |
There was a problem hiding this comment.
going to need more tests created to set these two values for proper validation.
one for default site url, another for the xloader override value. updating the fixture is so/so.
|
@duttonw yeah sure, will add tests, this is in draft as it is not ready for review by maintainers, I'm just doing a minimal validation of the approach ( I work with @wardi and @amercader at Link Digital ). |
We want to be able to communicate within a docker network without using the public site_url. This is a minimal proof of concept for achieving this.