Inventory ckan 2.8 builded datapusher #31
Conversation
…o inventory_ckan_2.8_builded_datapusher
…data99/inventory-app into inventory_ckan_2.8_builded_datapusher
|
I'm not able to add reviewrs here. @pjsharpe07 @adborden |
|
@avdata99 is this ready for review? It's still labeled WIP? |
avdata99
changed the title
I just fix the PR title. It's ready to analyze and merge |
|
@avdata99 so looking at the CKAN changes, it's making changes to the datapusher, right? Isn't the datapusher integrated into CKAN core at this point? |
| @@ -102,7 +103,7 @@ solr_url = http://solr:8983/solr/inventory | |||
| # original plugins | |||
| # ckan.plugins = usmetadata datajson datastore datapusher stats text_view recline_view googleanalyticsbasic | |||
|
|
|||
| ckan.plugins = googleanalyticsbasic datastore usmetadata datajson | |||
| ckan.plugins = googleanalyticsbasic datastore datapusher | |||
There was a problem hiding this comment.
Why the removal of usmetadata and datajson?
| # datapusher hotfix: CKAN has an error for running with docker. | ||
| # Fixed in master, need to port to CKAN 2.8.3 | ||
| # https://github.com/ckan/ckan/pull/5281/files | ||
| # -e git+https://github.com/avdata99/ckan@my_2.8.3#egg=ckan |
There was a problem hiding this comment.
Is your CKAN fix required before this PR is merged? Can you please push this to the GSA repo as a new branch datagov-inventory-2.8? I just added you as a collaborator https://github.com/GSA/ckan/invitations
| @@ -104,3 +104,13 @@ function test_datastore_request () { | |||
| return 1; | |||
| fi | |||
| } | |||
|
|
|||
| @test "datapusher working" { | |||
| datapusher_status=$(curl -X GET "http://datapusher:8000/status" | grep -o "push_to_datastore") | |||
There was a problem hiding this comment.
Please add the --fail flag so a non 200 status code results in curl failing. Also, no need to grep and capture the output, grep will return non-zero and fail the test if it doesn't find the pattern. No need for the if/else. The test is simply:
| datapusher_status=$(curl -X GET "http://datapusher:8000/status" | grep -o "push_to_datastore") | |
| curl --silent --fail "http://datapusher:8000/status" | grep push_to_datastore |
|
Hmm, @avdata99 FWIW, ckan/ckan@d834f17 looks like the wrong fix. If datapusher is hosted on a different Host, shouldn't we just set the Your fix changes site_url, which feels wrong to me. site_url should always be the CKAN URL (and |
@adborden From the point of view of the Datapusher (I mean, inside the Datapusher) you need to connect to the CKAN site URL in a different way when you are usin docker. localhost:5000 is a common site_url config but fails if you are usin docker. In that case you need ckan:5000 (or app:5000 or something like this). When CKAN talks to datapusher sends a ckan_url to allow datapusher to send data back to CKAN. Is just some lines bellow my changes. If we use public URLs for both services (CKAN and Datapusher) this is not a problem. This just fails when you are inside Docker. |
Using a built image of datapusher.
Added a test to check the service running.
I must fix CKAN because it fails to run datapusher in docker-compose. We will need to port this fix to CKAN 2.8.3. Maybe in a new issue.