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

Fix broken dev Redis default config #101

Merged
merged 1 commit into from
Feb 12, 2025
Merged

Conversation

AdamHawtin
Copy link
Contributor

@AdamHawtin AdamHawtin commented Feb 12, 2025

What is the context of this PR?

#76 Introduced an issue with the developer config where I incorrectly assumed REDIS_URL set in dev.py would properly populate the Redis configuration, instead it would incorrectly fall back on broken database caching which we do not want.

How to review

Check out the this branch
Run make compose-dev-up
Rename or remove any local .env or local.py config files
Try starting the wagtail app from the command line or IDE
The app should successfully start, not hit an error around database cache tables.

@AdamHawtin AdamHawtin added bug Something isn't working DX Developer eXperience labels Feb 12, 2025
@AdamHawtin AdamHawtin requested a review from a team as a code owner February 12, 2025 10:47
Copy link
Contributor

@kacperpONS kacperpONS left a comment

Choose a reason for hiding this comment

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

Works for me! thanks for the quick fix

kacperpONS added a commit that referenced this pull request Feb 12, 2025
@AdamHawtin AdamHawtin merged commit 39038e4 into main Feb 12, 2025
9 checks passed
@AdamHawtin AdamHawtin deleted the fix/dev-redis-config-issue branch February 12, 2025 12:00
Copy link
Contributor

@sanjeevz3009 sanjeevz3009 left a comment

Choose a reason for hiding this comment

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

Reviewed! Everything works as expected! Thanks @AdamHawtin !

kacperpONS added a commit that referenced this pull request Feb 24, 2025
* Add fix from PR #101

* Added the test

* Ignore pylint errors

* Make them pass

* Fix an issue with tests failing in headless and passing in headed mode

* Rewording to match steps pulled from main

* Formatting + rewording

* Revert a change to make tests pass

* A bunch of improvements

* Remove no longer needed file for theme page set up

* Remove duplicated step definition

* Add missing assertion

* Topic page test fix

* Added missing checks + where I got with testing preview (failing so commented out)

* Formatting - all green locally

* Test

* Attempt to fix headless failures

* Bump the wait time a bit

* Refactor the fix above into it's own step

* Uncommented the preview test

* Added a test for the draft functionality of the page

* Refactoring

* Renamed a test function

* Experiment - removed the delay in preview test

* ^ the same for the mandatory fields scenario 🤔

* Add back the delay for mandatory fields scenario + make step wording more precise

* Experiment - move the delay to the 'then' step of the mandatory fields scenario

* ^ revert the above

* Experiment - replace delay with wait_for_load_state

* ^ the same but after clicking the button

* Revert to using delay

* Test improvements

---------

Co-authored-by: Dan Braghiș <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working DX Developer eXperience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants