Add a "Validatable config" tests job to GitLab CI to help core evolve towards 100% validatability

To my own surprise, this pretty much worked on the first try 🤯

(Then again, I did already make a superset of all this work in config_inspector in 📌 Automated report on core config validatability Needs review , and that was much harder. So all the necessary pieces already existed.)

Impact

This would make the impact of 📌 Allow vocabularies to be validated via the API, not just during form submissions RTBC , 📌 Add validation constraints to announcements_feed.settings RTBC and ✨ New config schema data type: bytes Needs review (the three currently RTBC "config schema + validation" issues, one targeting all config entities of a certain type, one targeting a simple config object, one targeting a single config schema type) much easier to understand.

Reviews/likely concerns

I think there are a few obvious things to disagree with here:

1. 🤔 Is it okay for this job to install drush/drush? I could work around this, although there's no trivial way to install modules from the command line then.
2. 🤔 Is it okay for this job to install drupal/config_inspector? I could move the drush config:inspect --statistics logic into Drupal core, but that seems far worse, because it increases the maintenance burden of Drupal core?

But … the complexity here is entirely isolated to this single new core CI job. At any point in time can we easily remove this job. In fact, once the linked meta/plan issues are complete, I'd vote to remove this CI job. It's only a tool to help core's velocity and improve both contributor and committer DX.

This would also help make the Recipes initiative more likely to succeed, because more validatable config leads to more reliable recipes, which should lead to a stronger Recipes ecosystem.

I added some feedback to the MR. All minor, some of them are questions.

Feedback addressed!

It looks good from my end. I like that it is self-contained, as you intended from the beginning.
I'd mark it as RTBC but I think issues like bringing "drush" need to be reviewed by a core maintainer to see the feasibility of this (temporary) job, so I'll leave it as Needs Review.

Great work!

The one issue I see is that when we switch to main or open a new branch for core development, what happens when composer require drush/drush drupal/config_inspector starts failing because there is no compatible package yet? Given this runs on any change to a .module file then this is likely to make MRs on a new branch fail even if they are nothing to do with config schema.

But, perhaps we just deal with that when it happens by altering the job?

The one issue I see is that when we switch to main or open a new branch for core development, what happens when composer require drush/drush drupal/config_inspector starts failing because there is no compatible package yet?

1. config_inspector is already declaring 11.x compatibility and is explicitly testing daily with OPT_IN_TEST_NEXT_MAJOR.
2. Drush is a different matter indeed.

But, perhaps we just deal with that when it happens by altering the job?

+1 to this.

How about this for a pragmatic balance:

1. composer require … today exits with code 1 upon failure — we can append || exit 99
2. … then we could use allow_failure:exit_codes (docs) to specifically allow failure only for 99, but not for anything else

Tada, graceful degradation! 😄

@longwave Implemented what I proposed in #9 (but more explicitly/clearly) to address your concerns in #8: https://git.drupalcode.org/project/drupal/-/merge_requests/6700/diffs?co...

So now it should not even be necessary anymore to "just deal with it whenever that [Drush not compatible with next major Drupal version] happens".

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Merged in upstream, addressed feedback from @longwave & @alexpott.

Issue summary updated to reflect the crucial changes made to the original CI job to make it:

• more resilient (@longwave review)
• more complete (@alexpott review)

Neat!

Am seeing additional modules are being installed

[success] Successfully enabled: action, ban, basic_auth, book, config_translation, content_moderation, content_translation, datetime_range, field_layout, forum, help_topics, inline_form_errors, jsonapi, language, layout_builder, layout_discovery, locale, media, media_library, migrate, migrate_drupal, migrate_drupal_ui, mysql, pgsql, phpass, responsive_image, rest, sdc, serialization, settings_tray, statistics, syslog, telephone, tour, tracker, workflows, workspaces

Seems like a neat feature but would this eventually be a failing check when we reach 100%? Is there a way to have certain things skipped? Example deprecated modules

Seems like a neat feature but would this eventually be a failing check when we reach 100%?

How's that? We'd have to stay at 100%, then there'd be no failure. Status quo is proven to also not trigger test failures — see point 1 in the proposed resolution for an example.

Is there a way to have certain things skipped? Example deprecated modules

Sure, we can do that. When the need arises. Again: if this ever gets in the way of core development, we can just delete it!

In that case +1 but will leave in review for other eyes. Not sure I can mark it

Is there any additional testing I can do to mark this RTBC?

@smustgrave You can try marking more/fewer things as fully validatable and re-confirm that it all works as expected, like I did for the issue summary 😄

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

$ git merge origin/11.x
Auto-merging .gitlab-ci.yml
Merge made by the 'ort' strategy.
<snip>

👍

In install.core.inc line 964:
                                                                               
  The submitted value <em class="placeholder">sqlite</em> in the <em class="p  
  laceholder">Database type</em> element is not allowed. 

Not seen that one yet

#21: This MR cannot cause that. This must be a regression relating to switching to PHP 8.3 for all testing, or a random failure.

Ah, this is in the new CI job! Then obviously it is possible 😅 Though I have no idea what could've caused that… will investigate.

Looks like this is no longer working because of a core regression: 🐛 Regression: Error installing standard profile with sqlite Active .

Okay, switching from drush site-install to core/scripts/drupal install unblocked this. 😄 (I'd remove drush altogether, but there's no way to install modules using core/scripts/drupal today, that's being proposed in #3089277: Provide core CLI commands for the most common features of drush → .)

That means this is unblocked! 🥳

Don't mind marking if you want to remove that one change in core/config/schema/core.data_types.schema.yml

And question but do you want to lock down the version of config_inspector it pulls or fine with grabbing the latest?

Don't mind marking if you want to remove that one change in core/config/schema/core.data_types.schema.yml

Done.

And question but do you want to lock down the version of config_inspector it pulls or fine with grabbing the latest?

Grabbing latest is best. Let composer figure out the appropriate version for whichever core version. Because then we won't have to update the .gitlab-ci.yml to update to a newer version.

Makes sense!

I thought that we would need a similar check as per the cspell job where we take one SHA over another in some cases. But as that seems to work here maybe we can simplify that job as well? A task for another issue anyway.

I was also concerned about these steps:

git checkout $CI_MERGE_REQUEST_DIFF_BASE_SHA
composer require drush/drush drupal/config_inspector || exit 100
...
git checkout $CI_COMMIT_SHA

The composer require modifies the following files:

M	composer.json
M	composer.lock
M	composer/Metapackage/CoreRecommended/composer.json
M	composer/Metapackage/PinnedDevDependencies/composer.json
M	core/lib/Drupal/Component/Diff/composer.json

If any of these are different between the branches, the second git checkout may fail due to conflicts, as proven in this job: https://git.drupalcode.org/project/drupal/-/jobs/1023410

As we don't need Composer after installing Drush and Config Inspector maybe it is safe to git reset --hard before the second git checkout? Technically composer.json will be out of sync but it doesn't matter for the remainder of the job.

I was also concerned about these steps:
…
As we don't need Composer after installing Drush and Config Inspector maybe it is safe to use git checkout -f here?

Good suggestion! 👍 Implemented: https://git.drupalcode.org/project/drupal/-/merge_requests/6700/diffs?co... → passed ✅

Also updated your fork to verify it actually solves that edge case: https://git.drupalcode.org/project/drupal/-/merge_requests/6974/diffs?co... → passed ✅

Alright, let's give this a shot. Backported to 10.3.x to keep things in sync, I see no need to go back further with this one.

Committed and pushed 566b7d32e3 to 11.x and b27e31dc01 to 10.3.x. Thanks!

longwave → closed merge request !6974

longwave → closed merge request !6700

YAY!

This allowed me to close 📌 Create test that reports % of config entity types (and config schema types) that is validatable Postponed . And … I updated one of the-in-progress config validation issues, and sure enough, it works great: #3422872-10: Add validation constraints to contact.settings → 😊 Similarly, I checked a very important/huge scope issue ( 📌 Move code from the experimental SDC module to core Fixed ), I see that the job is running there (because it modified a .module file), and it's passing. So: it's not disrupting anything shortly after introduction at least. I'll be keeping an eye on this. Feel free to ping me directly if this ever gets in the way.

I'm excited about how this will empower 🌱 [meta] Config validation for a more reliable Drupal + reliable Recipes from the start Active to move faster! 🚀

This seems to be causing issues with MRs that are removing config entities

https://git.drupalcode.org/issue/drupal-3432874/-/jobs/1126875

Re-opening. Could we potentially count the raw number of validatable and unvalidatable config objects and then the test would need to be updated every time, but if we're removing a validatable config object, it would be easy to keep the test passing.

Or, should we just mark the job as allow_failure, so it still runs but doesn't block anything?

Not sure I've landed in the right place. I have two different MRs from two different issue. Both are changing or add schemas. And both are failing in this job. I have no idea what I'm doing wrong or what should I fix. I think the failure deserves a good error message in the job's log. Now is so opaque...

My cases:

• #2829919 📌 Either avoid or explicitly test binary encoding in default configuration Needs work > https://git.drupalcode.org/issue/drupal-2829919/-/jobs/1182735
• #2381293 ✨ Actions reordering on views bulk forms RTBC > https://git.drupalcode.org/issue/drupal-2381293/-/jobs/1149267

I'm in the same situation of @claudiu.cristea. I have a MR which modifies image field settings to add a property. How can I pass the test if the MR.json will always be different from HEAD.json?

.gitlab-ci.yml

# Compute statistics for coverage of validatable config for MR.
- vendor/bin/drush config:inspect --statistics > MR.json
# Output diff, but never fail the job.
- diff -u HEAD.json MR.json || true

#3325551 ✨ Add "Disable image resize" setting to image fields Needs review > https://git.drupalcode.org/issue/drupal-3325551/-/jobs/1209260

Re #40 catch agreed we should allow failures until the kinks are ironed out https://drupal.slack.com/archives/C1BMUQ9U6/p1711664511341669?thread_ts=...

Created an MR to allow that.

Hopefully helps the MRs

#42 I rebased and the validations least didn’t stop the MR that time.

Whatever happens next should be in a new issue.

Committed and pushed 722ab9b218 to 11.x and 539e16021f to 10.3.x. Thanks!

Let's do this and open a follow-up to make this fail again and improve the checking so it doesn't trigger false positives.

Created 📌 Fix issues with Validatable config job Active

Automatically closed - issue fixed for 2 weeks with no activity.