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

Add instructions for PERSIST_DIR to grist-local-testing README #1165

Merged
merged 2 commits into from
Aug 23, 2024

Conversation

dsagal
Copy link
Member

@dsagal dsagal commented Aug 17, 2024

Context

grist-local-testing README has a how-to-use section, which didn't work.

Proposed solution

Explain in README the extra step needed to make the minimal example work.

Has this been tested?

I tested the new instruction. There are no code changes here.

  • 👍 yes, I added tests to the test suite
  • 💭 no, because this PR is a draft and still needs work
  • 🙅 no, because this is not relevant here
  • 🙋 no, because I need help

@dsagal dsagal requested a review from Spoffy August 17, 2024 16:03
Copy link
Collaborator

@fflorent fflorent left a comment

Choose a reason for hiding this comment

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

Looks good to me. I made few suggestions that can be ignored.

Before running, create a directory where Grist will store its documents and settings:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpicking:

Suggested change
Before running, create a directory where Grist will store its documents and settings:
Before running the first time, create a directory where Grist will store its documents and settings:

Copy link
Collaborator

Choose a reason for hiding this comment

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

As an alternative, may we create the directory ourselves?

$ touch docker-compose-examples/grist-local-testing/persist/.gitkeep
$ git add -f docker-compose-examples/grist-local-testing/persist/.gitkeep # force the file to be indexed, ignoring .gitignore

And also this change to make PERSIST_DIR equal to ./persist in docker-compose.yml by default:

diff --git a/docker-compose-examples/grist-local-testing/docker-compose.yml b/docker-compose-examples/grist-local-testing/docker-compose.yml
index 028d4e0f..5ccf7399 100644
--- a/docker-compose-examples/grist-local-testing/docker-compose.yml
+++ b/docker-compose-examples/grist-local-testing/docker-compose.yml
@@ -3,6 +3,6 @@ services:
     image: gristlabs/grist:latest
     volumes:
       # Where to store persistent data, such as documents.
-      - ${PERSIST_DIR}/grist:/persist
+      - ${PERSIST_DIR:-./persist}/grist:/persist
     ports:
       - 8484:8484

What do you think?

…e; have docker-compose default to it; update README
@dsagal
Copy link
Member Author

dsagal commented Aug 21, 2024

Thanks for the suggestsions, @fflorent ! I used both, and also added some hopefully-helpful instructions to the README (you can see them rendered here).

@Spoffy , what do you think?

Copy link
Contributor

@Spoffy Spoffy left a comment

Choose a reason for hiding this comment

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

Looks good to me, a nice usability improvement!

@dsagal dsagal merged commit 8b48d1b into main Aug 23, 2024
10 of 11 checks passed
@dsagal dsagal deleted the update-docker-compose-readme branch August 23, 2024 21:50
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.

3 participants