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

BREAKING CHANGE: Switch from sqlite3 to MySQL by default #323

Merged
merged 1 commit into from
Aug 17, 2022

Conversation

tianon
Copy link
Member

@tianon tianon commented Aug 8, 2022

Fixes #310

The intent is to merge this breaking change to coincide with the 5.9 upstream Ghost release so that we have a clean place to point users back to (#310 (comment)).

Here's a full diff of the generated configuration files:

$ diff -u <(docker run --rm ghost:5 cat config.production.json) <(docker run --rm 02801f1260e3 cat config.production.json)
--- /dev/fd/63	2022-08-08 15:13:53.049985157 -0700
+++ /dev/fd/62	2022-08-08 15:13:53.053985192 -0700
@@ -4,12 +4,6 @@
     "port": 2368,
     "host": "::"
   },
-  "database": {
-    "client": "sqlite3",
-    "connection": {
-      "filename": "/var/lib/ghost/content/data/ghost.db"
-    }
-  },
   "mail": {
     "transport": "Direct"
   },
$ diff -u <(docker run --rm ghost:5-alpine cat config.production.json) <(docker run --rm 8cb6b6c0d885 cat config.production.json)
--- /dev/fd/63	2022-08-08 15:16:02.807350683 -0700
+++ /dev/fd/62	2022-08-08 15:16:02.807350683 -0700
@@ -4,12 +4,6 @@
     "port": 2368,
     "host": "::"
   },
-  "database": {
-    "client": "sqlite3",
-    "connection": {
-      "filename": "/var/lib/ghost/content/data/ghost.db"
-    }
-  },
   "mail": {
     "transport": "Direct"
   },

cc @ErisDS

One thing I'm not sure about is whether we should still include --db mysql in the ghost config line so that we get an explicit client: mysql in that configuration. Further, I think it would probably also make sense to include --dbhost mysql so that we also get host: mysql (instead of the default of "localhost" which isn't terribly useful in a container setup). Users would still need to supply their username/password/database, but at least this would be a starting point? Maybe even a default database of "ghost" or something? I don't want to make too many choices here that might end up in the same place as #310 😅

Verified

This commit was signed with the committer’s verified signature.
@ErisDS
Copy link
Collaborator

ErisDS commented Aug 10, 2022

The best advice I can give here really is to mirror whatever Ghost CLI does, or as close as you can get.

I don't mean to be vague, I can't remember what it does off the top of my head, but it's the best source of reference material for how Ghost likes things :)

The very easiest way to go and see a default setup is to go to https://marketplace.digitalocean.com/apps/ghost and spin up a droplet. It'll show you everything we do to make a "production ready" install.

Alternatively the code for the mysql extension in Ghost CLI lives here: https://github.com/TryGhost/Ghost-CLI/blob/main/extensions/mysql/index.js

@tianon
Copy link
Member Author

tianon commented Aug 10, 2022

No need to apologize, that's helpful! That helps me lean more towards keeping it light and trusting the Ghost CLI defaults instead (since that will help us stay in the right place over time too). 👍

I'm setting --db and --dbhost explicitly in ghost install just to bypass the "is MySQL and up running check" (since while building an image, we don't have any of that 😅), but leaving it out in the latter ghost config line so that we don't embed too much into that default config file. 👍

@tianon
Copy link
Member Author

tianon commented Aug 10, 2022

For my own ability to keep track of things -- what's missing for this PR:

  • a fix to the tests so we have passing CI again (probably spinning up MySQL and configuring to use it so we're testing it in the officially recommended way)
  • documentation for how to configure to use the (explicitly unsupported, which we need to call out) sqlite3 driver for an existing install

In this context, I personally love the "red banner" idea for sqlite3 installs since it'd make this a lot more obvious -- I think the only place that's different for container deploys than most other deploys is that users would be more likely to prefer environment variables for configuration than modifying JSON (so any documentation that the red banner links to might want to call out the environment variable configuration conversion specifically). 😅

Sorry, something went wrong.

@tianon
Copy link
Member Author

tianon commented Aug 11, 2022

I guess here is as good as anywhere for now -- for users who want to continue using the (explicitly unsupported!!!!) sqlite3 driver with their existing sqlite3 database, something like this in your environment should do the trick:

database__client: sqlite3
database__connection__filename: /var/lib/ghost/content/data/ghost.db

@tianon tianon merged commit 5de0cce into docker-library:master Aug 17, 2022
@tianon tianon deleted the breaking-change-mysql branch August 17, 2022 00:10
@tianon
Copy link
Member Author

tianon commented Aug 17, 2022

For users who find this because their deployments broke, see #323 (comment)

docker-library-bot added a commit to docker-library-bot/official-images that referenced this pull request Aug 17, 2022

Verified

This commit was signed with the committer’s verified signature.
Changes:

- docker-library/ghost@5de0cce: Merge pull request docker-library/ghost#323 from infosiftr/breaking-change-mysql
- docker-library/ghost@ad07986: Update to 5.9.4, ghost-cli 1.22.0
- docker-library/ghost@da02fe1: Update to 5.9.3, ghost-cli 1.22.0
- docker-library/ghost@e2ba2f8: Update to 5.9.1, ghost-cli 1.22.0
@muratcorlu
Copy link

muratcorlu commented Aug 17, 2022

Why do you release a breaking change with a minor version release? I was automatically updating my blogs with all new minor/patch versions but this broke all of them. Is it also possible to release a breaking change with a patch reelase? If so please don’t use semantic version numbers. Just number them independently.

@yosifkit
Copy link
Member

yosifkit commented Aug 17, 2022

This was supposed to have been applied in 5.0, but we didn't understand clearly enough to do this change then (and users were running into problems because we didn't change, #310). If you've been running any 5.x with a sqlite database in production mode, that is an unsupported configuration from 5.0.

@yosifkit yosifkit mentioned this pull request Aug 17, 2022
@muratcorlu
Copy link

Sorry, but that’s not a valid excuse. You are fixing a mistake with another mistake. When we had 5.0 release, I checked if I can just still use sqlite, and it just worked. Then I thought I’m safe until 6.0. If that was a mistake, you could fix it with 6.0. Otherwise what is the meaning of all these version numbers?

@tianon
Copy link
Member Author

tianon commented Aug 18, 2022

The meaning of the version numbers is defined by the Ghost project, not our packaging of it.

@muratcorlu
Copy link

Then please rely on Ghost project and don't break something with a minor version as they do. Otherwise we always need to check every Docker release notes if it breaks something. Even patch versions...

@yosifkit
Copy link
Member

We had to either fix the broken users (#310) by setting ghost up in the supported manner (and thus break some users when they upgrade to 5.9.x) or do nothing by leaving those users broken and the image configured in an explicitly unsupported manner. We chose to fix the image to be correct so that users also correct their unsupported deployments and we apologize that this misconfiguration affected users before and after the fix. The comment above gives users an "escape hatch" to keep using it in an unsupported configuration.

While we do our best to minimize breaking changes in the Dockerization, users should be testing every version of every image that they pull before deploying it to production.

@skptricks

This comment was marked as off-topic.

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.

database__connection__filename isn't blank and this causes an error with MySQL
5 participants