Feature/custom xloader site url rebased #243

duttonw · 2025-03-03T01:22:08Z

supersedes: pwalsh:feature/custom-xloader-site-url #234

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.

Update: There are 2 scenario's where XLoader communicates to CKAN

Scenario 1: When it downloads the resource file from CKAN ie: in _download_resource_data()
Scenario 2: When it updates the status of the XLoader job ie: in callback_xloader_hook()

If the config option ckanext.xloader.site_url is set then all communication from XLoader to CKAN will use this URL. In scenario 1, if it's not set then the ckan.site_url config option will be used unless the download ie: _download_resource_data() original_url is different to ckan_url (ie: ckan.site_url) and in that case the _download_resource_data() download will use the original_url but all other communication back to CKAN (ie: scenario 2) will use the ckan.site_url option

missed a callback_xloader_hook call, need to update result_url here too

hopefully getting closer

…e url replacement

duttonw · 2025-03-03T01:22:56Z

ckanext/xloader/plugin.py

@@ -76,14 +76,6 @@ def configure(self, config_):
        else:
            self.ignore_hash = False

-        for config_option in ("ckan.site_url",):


ckan.site_url is mandatory in 2.10+ and defaults to 127.0.0.1:5000 if not set, we don't need to config validate this.

…m which picks up job (which could have different ckan.ini config)

ckanext/xloader/config_declaration.yaml

ckanext/xloader/tests/test_jobs.py

ckanext/xloader/tests/test_utils.py

ckanext/xloader/utils.py

ThrawnCA · 2025-03-03T04:00:10Z

ckanext/xloader/tests/test_utils.py

+
+
+
+def test_modify_input_url_no_xloader_site():


Why is this a separate test instead of just another set of parameterised inputs?

it's also in the parameterised, but it may still be nice to keep seperate for 'clarity'

- provide better error messaging on incorrect api_key setup - user creation for testing to mimic real life 2.10+ environment chore: use cleaner pytest for setting variables

duttonw · 2025-03-03T06:22:31Z

.github/workflows/publish.yml

-      run: pytest --ckan-ini=test.ini --cov=ckanext.xloader --disable-warnings ckanext/xloader/tests
+    needs: validateVersion
+    name: Test
+    uses: ./.github/workflows/test.yml # Call the reusable workflow


Use test.yml so we don't need to duplicate how we test again.

duttonw · 2025-03-03T06:22:53Z

.github/workflows/test.yml

@@ -35,7 +35,7 @@ jobs:
            experimental: true  # master is unstable, good to know if we are compatible or not
      fail-fast: false

-    name: CKAN ${{ matrix.ckan-version }}
+    name: ${{ matrix.experimental && '**Fail_Ignored** ' || '' }} CKAN ${{ matrix.ckan-version }}


duplicate from another repo to give info that its 'ignoring' errors.

duttonw · 2025-03-03T06:23:17Z

.github/workflows/test.yml

      continue-on-error: ${{ matrix.experimental }}
      run: |
        ckan -c test.ini db init
+        ckan -c test.ini user add ckan_admin email=ckan_admin@localhost password="AbCdEf12345!@#%"
+        ckan -c test.ini sysadmin add ckan_admin
+        ckan config-tool test.ini "ckanext.xloader.api_token=$(ckan -c test.ini user token add ckan_admin xloader | tail -n 1 | tr -d '\t')"


configure proper xloader api token for test based on what would be going into the docker container.

duttonw · 2025-03-03T06:23:43Z

README.md

@@ -151,6 +151,7 @@ To install XLoader:
    execute jobs against the server:

        ckanext.xloader.api_token = <your-CKAN-generated-API-Token>
+        ckan config-tool test.ini "ckanext.xloader.api_token=$(ckan -c test.ini user token add ckan_admin xloader | tail -n 1 | tr -d '\t')"


todo: readme update on new ckan config options for custom xloader site url

duttonw · 2025-03-03T06:24:38Z

ckanext/xloader/config_declaration.yaml

-        required: false
+            apikey of the site_user.
+        default: 'NOT_SET'
+        required: true


dropped 2.9, set to mandatory and give it a default we can throw validation errors on. (i.e. stop the chicken and egg problem that you need a running ckan to create the api key to reference in the config)

duttonw · 2025-03-03T06:25:36Z

ckanext/xloader/tests/test_action.py

+    def test_xloader_user_api_token_from_config(self):
+        sysadmin = factories.SysadminWithToken()
+        apikey = sysadmin["token"]
+        with mock.patch.dict(toolkit.config, {'ckanext.xloader.api_token': apikey}):


this is to mimic what would be in the config. I ran out of time to work out how to look up a key inside pytest or hope that the db setup at the start was not 'cleansed'

For dynamic token values this approach does make sense.

duttonw · 2025-03-03T06:26:24Z

ckanext/xloader/utils.py

        return api_token
+    raise p.toolkit.ValidationError({u'ckanext.xloader.api_token': u'NOT_SET, please provide valid api token'})


hope this change catches miss configurations where xloader does it upload into the datastore but never 'completes' the job.

pwalsh and others added 9 commits March 3, 2025 11:09

Proof of concept for xloader site url

954ec75

Updates to config_declaration.yaml, jobs.py

73779dd

Changes for config_declaration.yaml and jobs.py as per PR

32884b9

Update jobs.py

b69b9c5

missed a callback_xloader_hook call, need to update result_url here too

Updates to jobs.py and utils.py

b93ac14

hopefully getting closer

Updates to jobs.py, utils.py and tests in test_jobs.py

944ce78

updates to action.py and utils.py

c8714ce

fix: add more unit tests

f12a988

fix: Streamline url replacement logic, introduce regex for ignore bas…

c040a78

…e url replacement

duttonw requested review from ThrawnCA and kowh-ai March 3, 2025 01:22

duttonw self-assigned this Mar 3, 2025

duttonw commented Mar 3, 2025

View reviewed changes

fix: revert to base wiring, as the url change occurs on runtime syste…

993cd00

…m which picks up job (which could have different ckan.ini config)

ThrawnCA requested changes Mar 3, 2025

View reviewed changes

duttonw mentioned this pull request Mar 3, 2025

Proof of concept for new config option: ckanext.xloader.site_url #234

Open

duttonw added 2 commits March 3, 2025 11:49

fix: remove validator on xloader.site_url as its not required

cbfc55a

chore: improve test coverage of "" and None inputs

d42bf99

ThrawnCA reviewed Mar 3, 2025

View reviewed changes

ThrawnCA approved these changes Mar 3, 2025

View reviewed changes

chore: cleanup 2.9 legacy items

ad5ea42

- provide better error messaging on incorrect api_key setup - user creation for testing to mimic real life 2.10+ environment chore: use cleaner pytest for setting variables

duttonw commented Mar 3, 2025

View reviewed changes

duttonw requested a review from ThrawnCA March 3, 2025 06:26

duttonw mentioned this pull request Mar 3, 2025

fix: add more unit tests pwalsh/ckanext-xloader#1

Closed

ThrawnCA approved these changes Mar 3, 2025

View reviewed changes

duttonw mentioned this pull request Mar 4, 2025

xloader still always pending #240

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/custom xloader site url rebased #243

Feature/custom xloader site url rebased #243

duttonw commented Mar 3, 2025

duttonw Mar 3, 2025

ThrawnCA Mar 3, 2025

duttonw Mar 3, 2025

duttonw Mar 3, 2025

duttonw Mar 3, 2025

duttonw Mar 3, 2025

duttonw Mar 3, 2025

duttonw Mar 3, 2025

duttonw Mar 3, 2025

ThrawnCA Mar 3, 2025

duttonw Mar 3, 2025

		return api_token
		raise p.toolkit.ValidationError({u'ckanext.xloader.api_token': u'NOT_SET, please provide valid api token'})

Feature/custom xloader site url rebased #243

Are you sure you want to change the base?

Feature/custom xloader site url rebased #243

Conversation

duttonw commented Mar 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment