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

Adds wagtail inventory to search wagtail pages based on the block type #818

Merged
merged 1 commit into from
Aug 14, 2019

Conversation

SaptakS
Copy link
Contributor

@SaptakS SaptakS commented Aug 5, 2019

Fixes #817

  • Installs wagtail inventory
  • Added environment parameters in docker-compose files to optionally run block inventory creation
  • Added instructions in readme to create a block inventory

@chigby
Copy link
Contributor

chigby commented Aug 6, 2019

@SaptakS I've run through the prod check locally using the commands which are at the top of the failing circle CI check:

pipenv run docker-compose -f prod-docker-compose.yaml build
pipenv run docker-compose -f prod-docker-compose.yaml up -d
while ! curl --output /dev/null --silent --head --fail http://$(docker-compose -f prod-docker-compose.yaml port nginx 8080); do sleep 1 && echo -n .; done;

This eventually led me to the conclusion (and it's not really obvious at all from the output on circle CI) that the nginx container is not starting and the while... line is looping forever. The nginx container is not starting because it depends on the django container starting, and the django container is not starting because the wagtailinventory package is not being installed. I eventually figured this out by checking the django logs locally with the command

docker-compose logs django

I think what's happening here is that the wagtail inventory package has been added to the dev-requirements.txt file and not to requirements.txt, the production file.

@SaptakS SaptakS force-pushed the add-wagtail-inventory branch 2 times, most recently from 2f835cd to d5f177c Compare August 6, 2019 17:59
@SaptakS
Copy link
Contributor Author

SaptakS commented Aug 6, 2019

@chigby thanks for pointing that out. Made the fix. Hoping the CI passes now.

@SaptakS
Copy link
Contributor Author

SaptakS commented Aug 6, 2019

Seems the CI is passing now.

@harrislapiroff
Copy link
Contributor

@msheiny this PR will require ./manage.py block_inventory to be run. This only needs to be run once (though it needs to be run again if we rename blocks). What do you think is the best way to do it? We could make running it part of the deploy scripts if we want; I believe it's idempotent, so it should be harmless to run every deploy even if we don't need it.

Copy link
Contributor

@harrislapiroff harrislapiroff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! I want to remove the configuration var for running block_inventory as part of docker-compose up in favor of always running it manually as documented. I also want to get @msheiny to weigh in on the best way to resolve my above question. Once those changes are made, this is ready to merge.

@@ -30,6 +30,9 @@ django_start() {
if [ "${DJANGO_CREATEDEVDATA:-no}" == "yes" ]; then
./manage.py createdevdata
fi
if [ "${DJANGO_CREATEINVENTORY:-no}" == "yes" ]; then
./manage.py block_inventory
fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmmm I'm not sure if I want to add this. I kind of already want to get rid of the CREATEDEVDATA option here...

@@ -47,6 +47,7 @@ services:
- postgresql
environment:
DJANGO_CREATEDEVDATA: "${DJANGO_CREATEDEVDATA:-no}"
DJANGO_CREATEINVENTORY: "${DJANGO_CREATEINVENTORY:-no}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above

@@ -34,6 +34,7 @@
'live_revision',
'search_image',
'blog_posts',
'page_blocks',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! Super interesting how Wagtail inventory works.

@@ -35,6 +35,7 @@ services:
DJANGO_ALLOWED_HOSTS: app
DJANGO_COLLECT_STATIC: "yes"
DJANGO_CREATEDEVDATA: "${DJANGO_CREATEDEVDATA:-no}"
DJANGO_CREATEINVENTORY: "${DJANGO_CREATEINVENTORY:-no}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above

@msheiny
Copy link

msheiny commented Aug 7, 2019

@msheiny this PR will require ./manage.py block_inventory to be run. This only needs to be run once (though it needs to be run again if we rename blocks). What do you think is the best way to do it? We could make running it part of the deploy scripts if we want; I believe it's idempotent, so it should be harmless to run every deploy even if we don't need it.

Can you elaborate what you mean by needs to be run again if we rename blocks ? Is there a way to wire this up to detect if blocks have changed ? I also see in the docs they list two commands:

Run migrations to create required database tables:

$ manage.py migrate wagtailinventory

Run a management command to initialize database tables with current pages:

$ manage.py block_inventory

Looks like that migrate command really needs to be run once but looks idempotent enough to be run everytime like our normal migrate executions. Heh the author actually responded in slack:

I’m happy to see some use of this package! The migrate command should be treated like any other Django/Wagtail migration. People often run migrations on each deploy so I’d recommend following best practices there. All migrations are “idempotent” in the sense that once Django records that they’ve been applied in the migrations table, they won’t be again. In the history of wagtail-inventory there haven’t been any DB schema changes (so far).

I see cfpb/wagtail-inventory#19 kinda echoes what you indicate. Looks like block_inventory is always deleting and recreating page inventory according to https://github.com/cfpb/wagtail-inventory/blob/master/wagtailinventory/management/commands/block_inventory.py#L16.

Here's what the author had to say here:

As for block_inventory, in theory, yes, you could run this on every deployment if you make schema changes. In practice if you only ever add new block types, you should never have to re-run block_inventory since page save() calls should update the table.
If/when you rename or remove blocks, though, you should re-run the command otherwise you’ll have out-of-date data in the results.
Your tolerance for how annoying this might be or how often to do this might depend on the number of blocks you have, frequency of changes, etc.

@SaptakS
Copy link
Contributor Author

SaptakS commented Aug 7, 2019

@harrislapiroff removed block_inventory commands from docker-compose yamls.

@harrislapiroff harrislapiroff merged commit 1de0a32 into master Aug 14, 2019
@harrislapiroff harrislapiroff deleted the add-wagtail-inventory branch August 14, 2019 16:54
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.

Install Wagtail Inventory
4 participants