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 Config Split and Updates Log #348

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ragnarkurmwunder
Copy link
Contributor

There is no ticket.
This PR is inspired by the need to integrate the Updates Log, and remove Warden.

Changes proposed in this PR:

  • Composer:
    • Add: Updates Log
    • Add: Config Split
  • Settings:
    • Add: Updates Log
    • Remove: Warden

How to test:

  1. See if the Config Split is under contrib modules
  2. See if the Updates Log is under contrib modules
$ ls -lad /app/web/modules/contrib/{config_split,updates_log}
drwxr-xr-x 1 www-data www-data 4096 Sep  3  2022 /app/web/modules/contrib/config_split
drwxr-xr-x 1 www-data www-data 4096 Mar 16 14:13 /app/web/modules/contrib/updates_log

Link to feature environment:
https://feature-updates-log--and--config-sp769.drupal-project.dev.wdr.io

@misterjoonas
Copy link
Member

misterjoonas commented Mar 22, 2023

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?
Also Monolog needs to be enabled.

"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!

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

@hkirsman hkirsman Oct 4, 2023

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.

@@ -42,17 +42,28 @@
'bower_components',
];

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

@hkirsman hkirsman Oct 4, 2023

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.

Copy link
Member

@tormi tormi left a 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",
Copy link
Member

Choose a reason for hiding this comment

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

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
Member

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"
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?

@@ -42,17 +42,28 @@
'bower_components',
];

// $settings['updates_log_site'] = 'my-project-name';
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.

// $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.

@tormi
Copy link
Member

tormi commented Apr 18, 2024

This requires removal of basic profile, see also #403.

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.

None yet

4 participants