-
-
Notifications
You must be signed in to change notification settings - Fork 227
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
✨ Added support for multiple built-in themes #1784
Conversation
lib/commands/update.js
Outdated
// If upgrading from a version < 5.63.0, we need to symlink masthead so it is installed and available to update | ||
if (semver.lt(instance.version, '5.63.0') && semver.gte(version, '5.63.0')) { | ||
symlinkSync( | ||
path.join(process.cwd(), 'current', 'content', 'themes', 'masthead'), | ||
path.join(process.cwd(), 'content', 'themes', 'masthead') | ||
); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be done as a migration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I tried this out because I agree it's a bit awkward to put this code here, but it didn't do what I need it to 😔. The migrations file you linked to are run depending on the CLI version that installed the instance of Ghost, and I need to create this symlink depending on the Ghost instance version. Basically, the migrations you're referring to are dependent on how Ghost was installed, rather than which version of Ghost was installed.
Basically, Ghost 5.62.0
doesn't include this theme, but 5.63.0+
will. We could of course check the Ghost version inside the migration and that would work in most cases (e.g. when upgrading sites on Ghost (Pro)), but you could theoretically use the latest version of the CLI (e.g. 1.24.4
once this change is released) to install an older version of Ghost (5.62.0
or earlier) and then use the same version of the CLI to update to 5.63.0
. In this case, the migration wouldn't run because the CLI version hasn't changed and therefore the theme wouldn't be installed.
I'll probably break this block into its own task because I agree it doesn't really fit inside the link
task, but I don't think using the existing migrations 'framework' works in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, yeah, the main issue I see is that older versions of Ghost are "vulnerable" to invalid symlinks (the stat call fails but isn't handled). If they supported invalid symlinks, you could create a symlink regardless of the Ghost version.
It would be great if Ghost's migrations could handle this, but you'd have to assume that everyone is using the CLI, and hardcode current
into the path 🤮
Another option is to extend the migrations framework to also support beforeInstance
. You would need to add rollback support, though. Then again, rollback support is required regardless.
e: On the other side, I don't see any issues with a missing symlink in new versions of Ghost, since the core declares the minimum CLI version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vikaspotluri123 I spent some time refactoring this today to address some of your concerns, and I think the result is much improved from my first attempt:
- The install command no longer checks the version number — it simply creates a symlink for all themes that are shipped with Ghost. So for versions < 5.67.0, it will only link
casper
because that's the only theme that ships with Ghost, for 5.67.0+ it will linkcasper
andsource
, and eventually when we removecasper
it will only installsource
. - The update command uses basically the same logic — it will create symlinks for any theme that installs with Ghost, so when updating to 5.67.0 it will create a symlink for
source
and in the case of a rollback, it will remove any symlinks that are broken in the process of the rollback (e.g. when rolling back from 5.67.0 it will remove the symlink tosource
because it is not present in 5.66.1). - I also added a migration to Ghost that will be included in 5.67.0 which does nothing when upgrading, but when rolling back to < 5.67.0 it will check if
source
is the active_theme and update it tocasper
if so.
These changes paired with a migration in Ghost to set the active theme to casper only when rolling back to < 5.67.0 and source
is the current theme should address all your concerns, and I think it's a more elegant solution than manually checking the version at various points in the CLI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall! Left a few comments and suggestions 😊
4a28f49
to
34d1113
Compare
refs TryGhost/Product#3510 - This change is to accommodate a change in Ghost planned to be released in `v5.67.0`, which changes the default theme from Casper to Source - Updated install and update commands to create symlinks for any theme that is shipped alongside Ghost (e.g. any theme in `ghost/core/content/themes`) - When rolling back with `ghost update --rollback`, any symlinks that are broken in the process will be removed (e.g. when rolling back from `5.67.0` to `5.66.1`, the symlink to `source` would be broken so it is removed)
Source
as the default theme
Co-authored-by: Vikas Potluri <vikaspotluri123.github@gmail.com>
Source
as the default themerefs #1784 (comment) - as per feedback in referenced link
refs TryGhost/Product#3510
ghost/core/content/themes
)ghost update --rollback
, any symlinks that are broken in the process will be removed