Comments from group review

.circleci/config.yml

@@ -7,7 +7,7 @@ jobs:
       - checkout
       - run:
           name: lint code
-          command: make lint
+          command: make COMPOSE_FILE=docker-compose.legacy.yml lint
   test_local_docker:
     working_directory: ~/ckanext-datagovtheme
     machine: true
@@ -16,8 +16,8 @@ jobs:
       - run:
           name: Start CKAN
           command: |
-            docker-compose build
-            docker-compose up -d
+            docker-compose -f docker-compose.legacy.yml build
+            docker-compose -f docker-compose.legacy.yml up -d
       - run:
           name: install dockerize
           command: wget https://github.com/jwilder/dockerize/releases/download/$DOCKERIZE_VERSION/dockerize-linux-amd64-$DOCKERIZE_VERSION.tar.gz && sudo tar -C /usr/local/bin -xzvf dockerize-linux-amd64-$DOCKERIZE_VERSION.tar.gz && rm dockerize-linux-amd64-$DOCKERIZE_VERSION.tar.gz
@@ -29,9 +29,9 @@ jobs:
       - run:
           name: Test extension
           command: |
-            docker-compose logs db
-            docker-compose logs ckan
-            docker-compose exec ckan /bin/bash -c "nosetests --ckan --with-pylons=src/ckan/test-catalog-next.ini src_extensions/datagovtheme/ckanext/datagovtheme/tests/nose"
+            docker-compose -f docker-compose.legacy.yml logs db
+            docker-compose -f docker-compose.legacy.yml logs ckan
+            docker-compose -f docker-compose.legacy.yml exec ckan /bin/bash -c "nosetests --ckan --with-pylons=src/ckan/test-catalog-next.ini src_extensions/datagovtheme/ckanext/datagovtheme/tests/nose"
   build_ckan_28:
     working_directory: ~/ckanext-datagovtheme
     machine:

.github/workflows/ckan.yml

@@ -1,7 +1,7 @@
 ---
 # https://github.com/ckan/ckanext-harvest/blob/3c83f41131c99042e7a44da858515c5563faba01/.github/workflows/test.yml
 name: Tests
-on: [push, pull_request]
+on: [pull_request]
 env:
   CODE_COVERAGE_THRESHOLD_REQUIRED: 28
 

Makefile

@@ -8,24 +8,24 @@ clean: ## Clean workspace and containers
 	find . -name *.pyc -delete
 	CKAN_VERSION=$(CKAN_VERSION) docker-compose -f $(COMPOSE_FILE) down -v --remove-orphan
 
-lint: ## Lint the code
+lint: ## Lint the code (python 3 only)
 	pip install pip==20.3.3
 	pip install flake8
 	flake8 . --count --show-source --statistics --exclude ckan
 
-test: ## Run tests in an existing container
+test-legacy: ## Run legacy tests in an existing container
 	@# TODO wait for CKAN to be up; use docker-compose run instead
-	docker-compose exec ckan /bin/bash -c "nosetests --ckan --with-pylons=src/ckan/test-catalog-next.ini src_extensions/datagovtheme/ckanext/datagovtheme/tests/nose"
+	docker-compose -f $(COMPOSE_FILE) exec ckan /bin/bash -c "nosetests --ckan --with-pylons=src/ckan/test-catalog-next.ini src_extensions/datagovtheme/ckanext/datagovtheme/tests/nose"
 
-test-new: ## Run "new" style tests
-	CKAN_VERSION=$(CKAN_VERSION) docker-compose -f docker-compose.new.yml run --rm app ./test.sh
+test: ## Run extension tests
+	CKAN_VERSION=$(CKAN_VERSION) docker-compose -f $(COMPOSE_FILE) run --rm app ./test.sh
 
 up: ## Start the containers
 	CKAN_VERSION=$(CKAN_VERSION) docker-compose -f $(COMPOSE_FILE) up
 
 
 .DEFAULT_GOAL := help
-.PHONY: clean help lint test up
+.PHONY: clean help lint test test-legacy up
 
 # Output documentation for top-level targets
 # Thanks to https://marmelab.com/blog/2016/02/29/auto-documented-makefile.html

README.md

@@ -104,12 +104,14 @@ The existing development environment assumes a full catalog.data.gov test setup.
 it difficult to develop and test against new versions of CKAN (or really any
 dependency) because everything is tightly coupled and would require us to
 upgrade everything at once which doesn't really work. A new make target
-`test-new` is introduced with a new docker-compose file.
+`test-legacy` is introduced with a new `docker-compose.legacy.yml` file in order
+to run tests in the legacy docker environment.
 
 The "new" development environment drops as many dependencies as possible. It is
 not meant to have feature parity with
-[GSA/catalog.data.gov](https://github.com/GSA/catalog.data.gov/). Tests should
-mock external dependencies where possible.
+[GSA/catalog.data.gov](https://github.com/GSA/catalog.data.gov/) or
+[GSA/inventory-app](https://github.com/GSA/inventory-app/). Tests should mock
+external dependencies where possible.
 
 In order to support multiple versions of CKAN, or even upgrade to new versions
 of CKAN, we support development and testing through the `CKAN_VERSION`
@@ -119,10 +121,13 @@ environment variable.
     $ make CKAN_VERSION=2.9 test-new
 
 
-Other docker-compose make targets work in both new and old environments through
-the `COMPOSE_FILE` make variable.
+Other docker-compose make targets work in both new and legacy environments through
+the `COMPOSE_FILE` make variable. To test the legacy environment:
 
-    $ make COMPOSE_FILE=docker-compose.new.yml clean
+    $ make COMPOSE_FILE=docker-compose.legacy.yml up
+    $ make COMPOSE_FILE=docker-compose.legacy.yml test-legacy
+
+_Note: the test-legacy target only works in the legacy docker environment._
 
 Variable | Description | Default
 -------- | ----------- | -------

ckanext/datagovtheme/fanstatic_library/webassets.yml

@@ -39,4 +39,6 @@ spatial_query:
     - scripts/vendor/leaflet/leaflet.js
     - scripts/vendor/leaflet.draw/leaflet.draw.js
     - scripts/spatial_query.js
+    # TODO can this custom location search be replaced by ckanext-spatial? Can
+    # this be pushed upstream to ckanext-spatial as an enhancement?
     - scripts/location_autocomplete.js

ckanext/datagovtheme/helpers.py

@@ -29,6 +29,10 @@
 ckan_tmp_path = '/var/tmp/ckan'
 
 # TODO figure out where this belongs
+# This is used in multiple extensions, including ckanext-geodatagov. It seems
+# like it's just data and could be exposed differently. We could also provide a
+# fallback, so if ckanext-geodatagov is available, use the data from that,
+# otherwise fallback to an alternative.
 RESOURCE_MAPPING = {
     # ArcGIS File Types
     'esri rest': ('Esri REST', 'Esri REST API Endpoint'),
@@ -486,6 +490,7 @@ def get_reference_date(date_str):
 
 
 # https://github.com/ckan/ckanext-spatial/blob/011008b9c5c4bf58ddd401c805328a9928bbe4ea/ckanext/spatial/helpers.py#L35
+# This doesn't seem specific to ckanext-spatial. Maybe this should move into ckanext-harvest or ckan proper?
 def get_responsible_party(value):
     '''
         Gets a responsible party extra created by the harvesters and formats it

ckanext/datagovtheme/tests/test_datagovtheme.py

@@ -5,7 +5,13 @@
 '''
 from builtins import object
 
+import pytest
 
+
+# The /dataset page uses get_pkg_dict_extra which depends on HarvestObject,
+# hence the harvest extension. Include it for these tests.
+@pytest.mark.ckan_config('ckan.plugins', 'harvest datagovtheme')
+@pytest.mark.use_fixtures('with_plugins')
 class TestDatagovthemeServed(object):
     '''Tests for the ckanext.datagovtheme.plugin module.'''
 

ckanext/datagovtheme/tests/test_notes.py

@@ -5,7 +5,10 @@
 from ckantoolkit.tests import factories
 
 
-@pytest.mark.usefixtures('clean_db', 'clean_index')
+# The /dataset page uses get_pkg_dict_extra which depends on HarvestObject,
+# hence the harvest extension. Include it for these tests.
+@pytest.mark.ckan_config('ckan.plugins', 'harvest datagovtheme')
+@pytest.mark.use_fixtures('with_plugins', 'clean_db', 'clean_index')
 class TestNotes(object):
     '''Tests for the ckanext.datagovtheme.plugin module.'''
 

ckanext/datagovtheme/tests/test_ui_data.py

@@ -5,7 +5,10 @@
 import pytest
 
 
-@pytest.mark.usefixtures('clean_db', 'clean_index')
+# The /dataset page uses get_pkg_dict_extra which depends on HarvestObject,
+# hence the harvest extension. Include it for these tests.
+@pytest.mark.ckan_config('ckan.plugins', 'harvest datagovtheme')
+@pytest.mark.use_fixtures('with_plugins', 'clean_db', 'clean_index')
 class TestSearchFilters():
 
     @pytest.fixture(autouse=True)

docker-compose.legacy.yml

@@ -0,0 +1,43 @@
+---
+# This docker environment is considered legacy. This uses images built from
+# catalog in order to test in an environment very close. Unfortunately, it
+# creates an external dependency and breaks often and unexpectedly. This
+# approach introduces additional variables in the tests (e.g. many additional
+# CKAN extensions) that we are not trying to test and end up causing false
+# positives or false negatives. The new approach is less coupled to catalog and
+# instead uses "vanilla" CKAN images as much as possible, with as few
+# dependencies as possible.
+# TODO delete this after catalog is running CKAN 2.9
+version: "3.2"
+services:
+  ckan:
+    image: datagov/catalog.data.gov:latest
+    env_file:
+      - ckan_env
+    depends_on:
+      - db
+      - solr
+      - redis
+    ports:
+      - "5000:5000"
+    volumes:
+      - ckan_storage:/var/lib/ckan
+      - .:/srv/app/src_extensions/datagovtheme
+  db:
+    image: datagov/catalog.data.gov.db
+    ports:
+      - "5432:5432"
+    expose:
+      - "5432"
+  solr:
+    image: datagov/catalog.data.gov.solr
+    restart: always
+    expose:
+      - "8983"
+  redis:
+    image: redis:alpine
+
+volumes:
+  ckan_storage:
+  pg_data:
+  solr_data:

docker-compose.yml

@@ -1,33 +1,41 @@
-version: "3.2"
+---
+version: "3.7"
 services:
-  ckan:
-    image: datagov/catalog.data.gov:latest
-    env_file:
-      - ckan_env
+  app:
+    image: datagov/ckanext-datagovtheme:${CKAN_VERSION} # ensures docker-compose will rebuild the right image in case we change CKAN_VERSION
+    build:
+      context: .
+      args:
+        CKAN_VERSION: ${CKAN_VERSION}
+    environment:
+      CKAN_SQLALCHEMY_URL: postgresql://ckan_default:pass@db/ckan_test
+      CKAN_DATASTORE_WRITE_URL: postgresql://datastore_write:pass@db/datastore_test
+      CKAN_DATASTORE_READ_URL: postgresql://datastore_read:pass@db/datastore_test
+      CKAN_SOLR_URL: http://solr:8983/solr/ckan
+      CKAN_REDIS_URL: redis://redis:6379/
+      CKAN_DATAPUSHER_URL: http://localhost:8080/  # datapusher is not really enabled
+      CKAN__PLUGINS: harvest datagovtheme
+    working_dir: /app
+    ports:
+      - "5000:5000"
     depends_on:
       - db
-      - solr
       - redis
-    ports:
-      - "5000:5000"
+      - solr
     volumes:
-      - ckan_storage:/var/lib/ckan
-      - .:/srv/app/src_extensions/datagovtheme
+      - .:/app
   db:
-    image: datagov/catalog.data.gov.db
-    ports:
-      - "5432:5432"
-    expose:
-      - "5432"
-  solr:
-    image: datagov/catalog.data.gov.solr
-    restart: always
-    expose:
-      - "8983"
+    image: ckan/ckan-postgres-dev:${CKAN_VERSION}
+    environment:
+      POSTGRES_USER: postgres
+      POSTGRES_PASSWORD: postgres
+      POSTGRES_DB: postgres
+    healthcheck:
+      test: ["CMD", "pg_isready"]
+      interval: 10s
+      timeout: 5s
+      retries: 5
   redis:
-    image: redis:alpine
-
-volumes:
-  ckan_storage:
-  pg_data:
-  solr_data:
+    image: redis
+  solr:
+    image: ckan/ckan-solr-dev:${CKAN_VERSION}

test.ini

@@ -11,7 +11,7 @@ error_email_from = paste@localhost
 use = config:/srv/app/src/ckan/test-core.ini
 ckan.site_title = My Test CKAN Site
 ckan.site_description = A test site for testing my CKAN extension
-ckan.plugins = harvest datagovtheme
+ckan.plugins = datagovtheme
 ckan.legacy_templates = no
 sqlalchemy.url = postgresql://ckan:pass@db/ckan
 solr_url = http://solr:8983/solr/ckan

test.sh

@@ -8,13 +8,6 @@
 set -o errexit
 set -o pipefail
 
-# configure environment for tests
-export CKAN_SQLALCHEMY_URL=postgresql://ckan_default:pass@db/ckan_test
-export CKAN_DATASTORE_WRITE_URL=postgresql://datastore_write:pass@db/datastore_test
-export CKAN_DATASTORE_READ_URL=postgresql://datastore_read:pass@db/datastore_test
-export CKAN_SOLR_URL=http://solr:8983/solr/ckan
-export CKAN_REDIS_URL=redis://redis:6379/1
-
 # Wrapper for paster/ckan.
 # CKAN 2.9 replaces paster with ckan CLI. This wrapper abstracts which comand
 # is called.
@@ -33,6 +26,5 @@ function ckan_wrapper () {
 }
 
 ckan_wrapper --plugin=ckan db init
-ckan_wrapper --plugin=ckanext-harvest harvester initdb
 
 pytest --ckan-ini=test.ini --cov=ckanext.datagovtheme --disable-warnings ckanext/datagovtheme/tests