-
Notifications
You must be signed in to change notification settings - Fork 6
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 Config Split and Updates Log #348
base: main
Are you sure you want to change the base?
Conversation
There could be some instructions in README in which environments the updates_log module should be enabled. Basically main and production, but not in feature branches. I hope they are separated by env variables in Silta? |
"drupal/core-composer-scaffold": "^10.0", | ||
"drupal/core-recommended": "^10.0", | ||
"drupal/monolog": "^3.0@beta", | ||
"drupal/simplei": "^2.0@beta", | ||
"drush/drush": "^11.4", | ||
"vlucas/phpdotenv": "^5.4", | ||
"webflo/drupal-finder": "^1.2", | ||
"wunderio/drupal-ping": "^2.4" | ||
"wunderio/drupal-ping": "^2.4", |
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 we can also remove Warden because it's new projects that start using it? @tormi
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.
Yes!
"drupal/core-composer-scaffold": "^10.0", | ||
"drupal/core-recommended": "^10.0", | ||
"drupal/monolog": "^3.0@beta", | ||
"drupal/simplei": "^2.0@beta", | ||
"drush/drush": "^11.4", | ||
"vlucas/phpdotenv": "^5.4", | ||
"webflo/drupal-finder": "^1.2", | ||
"wunderio/drupal-ping": "^2.4" | ||
"wunderio/drupal-ping": "^2.4", | ||
"wunderio/updates_log": "^2" |
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 noticed there's this profile web/profiles/custom/basic/basic.info.yml Is it supposed to be used during fresh Drupal projects? Should we enable the updates_log there? It had one bug that it could not find CKEditor atm.
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.
This has been on hold for a while. My position is that we should remove the basic
profile and switch to minimal
with config in config/sync
folder. Then we can export config changes more easily. Also, we can implement config_split
in a way it's been done in our projects.
@@ -42,17 +42,28 @@ | |||
'bower_components', | |||
]; | |||
|
|||
// $settings['updates_log_site'] = 'my-project-name'; |
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.
Shouldn't there also be section in readme for getting this up and running?
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.
@ragnarkurmwunder, I propose we leave updates_log_site
and updates_log_env
out from this template as it's targeting projects in Silta and the updates_log
features are documented in it's repository.
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.
Good stuff, but this needs some work.
@@ -19,14 +19,16 @@ | |||
"composer/installers": "^2.1", | |||
"cweagans/composer-patches": "^1.7", | |||
"drupal/admin_toolbar": "^3.3", | |||
"drupal/config_split": "^2.0@RC", |
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.
This can be replaced now with 2.0
, https://www.drupal.org/project/config_split/releases/2.0.0
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.
We're introducing config_split
component here, but without any settings & configs. I think we should at least add relevant settings in settings.php
and related documentation.
"drupal/core-composer-scaffold": "^10.0", | ||
"drupal/core-recommended": "^10.0", | ||
"drupal/monolog": "^3.0@beta", | ||
"drupal/simplei": "^2.0@beta", | ||
"drush/drush": "^11.4", | ||
"vlucas/phpdotenv": "^5.4", | ||
"webflo/drupal-finder": "^1.2", | ||
"wunderio/drupal-ping": "^2.4" | ||
"wunderio/drupal-ping": "^2.4", |
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.
Yes!
"drupal/core-composer-scaffold": "^10.0", | ||
"drupal/core-recommended": "^10.0", | ||
"drupal/monolog": "^3.0@beta", | ||
"drupal/simplei": "^2.0@beta", | ||
"drush/drush": "^11.4", | ||
"vlucas/phpdotenv": "^5.4", | ||
"webflo/drupal-finder": "^1.2", | ||
"wunderio/drupal-ping": "^2.4" | ||
"wunderio/drupal-ping": "^2.4", | ||
"wunderio/updates_log": "^2" |
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.
Let's continue using MAJOR.MINOR
syntax because it's also default in drupal.org release notes (f.ex composer require 'drupal/config_split:^2.0'
in https://www.drupal.org/project/config_split/releases/2.0.0.
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 noticed there's this profile web/profiles/custom/basic/basic.info.yml Is it supposed to be used during fresh Drupal projects? Should we enable the updates_log there? It had one bug that it could not find CKEditor atm.
Very good question, @hkirsman! I think we could even remove this custombasic
install profile since we don't use install profiles in our projects. And the Drupal Recipes Initiative sums up the reasons nicely:
Historically this functionality has been served by install profiles and distributions, but both of those iterations of the problem have some flaws:
They are difficult to keep updated
They cannot be added after starting your project
They can't be removed easily
They can't be mixed and matched with other sets of functionality.
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.
We're using recipes in our https://github.com/wunderio/next-drupal-starterkit template and it seems to be working well. Perhaps we should introduce it here as well?
@@ -42,17 +42,28 @@ | |||
'bower_components', | |||
]; | |||
|
|||
// $settings['updates_log_site'] = 'my-project-name'; |
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.
@ragnarkurmwunder, I propose we leave updates_log_site
and updates_log_env
out from this template as it's targeting projects in Silta and the updates_log
features are documented in it's repository.
// $settings['updates_log_env'] = getenv('ENVIRONMENT_NAME'); | ||
// In Silta it is set by default as ENVIRONMENT_NAME | ||
|
||
// $settings['updates_log_disabled'] = TRUE; |
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.
Let's not bring that double negation variable into the template! Instead, let's implement config_split
properly either via config
or recipes
.
This requires removal of |
There is no ticket.
This PR is inspired by the need to integrate the Updates Log, and remove Warden.
Changes proposed in this PR:
How to test:
Link to feature environment:
https://feature-updates-log--and--config-sp769.drupal-project.dev.wdr.io