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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+16 −3
Open

Add Config Split and Updates Log #348

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
219a73b
Modules: Config Split, Updates Log: Add
ragnarkurmwunder Mar 21, 2023
28870fe
Settings: Update. Remove Warden, Add UpdatesLog.
ragnarkurmwunder Mar 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,16 @@
"composer/installers": "^2.1",
"cweagans/composer-patches": "^1.7",
"drupal/admin_toolbar": "^3.3",
"drupal/config_split": "^2.0@RC",
Copy link
Member

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

Copy link
Member

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",
Copy link
Contributor

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

Copy link
Member

Choose a reason for hiding this comment

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

Yes!

"wunderio/updates_log": "^2"
Copy link
Contributor

@hkirsman hkirsman Oct 4, 2023 •
edited
Loading

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.

2023-10-04_13-30

2023-10-04_13-31

@tormi

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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?

},
"require-dev": {
"drupal/core-dev": "^10.0",
Expand Down
15 changes: 13 additions & 2 deletions web/sites/default/settings.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,17 +42,28 @@
'bower_components',
];

// $settings['updates_log_site'] = 'my-project-name';
Copy link
Contributor

@hkirsman hkirsman Oct 4, 2023 •
edited
Loading

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?

@tormi @ragnarkurmwunder

Copy link
Member

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.

// In Silta it is set by default as github repo name.

// $settings['updates_log_env'] = getenv('ENVIRONMENT_NAME');
// In Silta it is set by default as ENVIRONMENT_NAME

// $settings['updates_log_disabled'] = TRUE;
Copy link
Member

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.

// In case config_split is missing.

// Environment-specific settings.
$env = $_ENV['ENVIRONMENT_NAME'];
switch ($env) {
case 'production':
$settings['simple_environment_indicator'] = 'DarkRed Production';
// Warden settings.
$config['warden.settings']['warden_token'] = $_ENV['WARDEN_TOKEN'];
// $settings['updates_log_disabled'] = FALSE;
// In case config_split is missing.
break;

case 'main':
$settings['simple_environment_indicator'] = 'DarkBlue Stage';
// $settings['updates_log_disabled'] = FALSE;
// In case config_split is missing.
break;

case 'local':
Expand Down