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

Improve Configuration Validation [ spike ] #151

Open
adamzimmermann opened this issue Jul 17, 2023 · 8 comments
Open

Improve Configuration Validation [ spike ] #151

adamzimmermann opened this issue Jul 17, 2023 · 8 comments
Assignees

Comments

@adamzimmermann
Copy link
Contributor

adamzimmermann commented Jul 17, 2023

Description

During drush deploy update hooks are ran and then configuration is imported.

This is great for deploying to production, but it makes it very hard to assert that the configuration in code matches the most up to date state of the database.

A module update done via one of our automated tools may have an update hook that changes configuration. The subsequent configuration import will likely blow those changes away, and then our configuration checking tool in Usher will not find an issue.

Questions / Areas of Uncertainty

  • How can we check the state of configuration after database updates, but before configuration import?
  • Do we need to override the drush deploy command or create or own version that extends it for CI usage?

Outcome

We can confidently validate that the configuration in code has the latest values, even if an automated dependency update includes changes to configuration via an update hook.

Additional Context

We will need a fully functioning/bootstrapped version of Drupal in a CI environment to validate this.

Existing Config Validation Code:

This might be worth exploring:

@goranmiric and I had a good discussion about this recently somewhere, but I can't remember where. 🤔

Drupal Issues

Related Blog Posts

@markdorison
Copy link
Contributor

A module update done via one of our automated tools may have an update hook that changes configuration. The subsequent configuration import will likely blow those changes away

If a new config item is added to Drupal core or contrib, and we run a config:import as part of drush deploy, I don't believe that config item would/should be overridden, unless something has changed in Drupal 10. Isn't this the scenario we created the "drupal-status" check to catch?

How can we check the state of configuration after database updates, but before configuration import?

Checking before config:import would fail (inaccurately) whenever we commit changes to a site's configuration.

@goranmiric
Copy link
Collaborator

@markdorison I just remembered one example.
The latest issue: Drupal 10.1 shipped with a new module phpass

  • core hook_update enabled the module
  • drush cim disabled it
    After that everything was in sync but actually we had an issue.

@markdorison
Copy link
Contributor

@goranmiric Thanks for that example! That makes sense.

@adamzimmermann
Copy link
Contributor Author

How can we check the state of configuration after database updates, but before configuration import?

Checking before config:import would fail (inaccurately) whenever we commit changes to a site's configuration.

Good point. 🙈

🤔 So we need to detect issues like the one @goranmiric described above, but not fail the more common scenario of us simply changing configuration files.

I need to ponder this.

@adamzimmermann
Copy link
Contributor Author

So what we really need to do is:

  1. run a config import
  2. run update hooks
  3. then check for a config diff

That does not match the order used for a production deployment or what drush deploy does, so wherever we run this would need to be isolated and be an additional process, not something we simply do alternatively in Tugboat or somewhere else.

That significantly adds to the overhead unless we can think of something clever. 😭

We almost need two databases to handle the two different states. 😬


  • Alternatively could we check to see if the deployment will run any update hooks with updatedb:status
  • If it will, just simply flag it some special way?
  • Or grep the update hooks for some pattern or method call?
  • Maybe just include a link to the diff in a GitHub comment?

@markdorison
Copy link
Contributor

@adamzimmermann and I talked about this issue this morning and he has updated the issue description with links to some relevant Drupal core issues. I would also specifically note a comment I just made on this issue: If an update hook modifies configuration, then old configuration is imported, the changes made by the update hook are forever lost.

This issue recently affected us with the release of Drupal 10.1. #1845004: Replace custom password hashing library with PHP password_hash() included an update hook to enable the new phpass core module. Once that update hook ran, the module was enabled only to be disabled when config:import was subsequently run.

We have automated tests that check the status of Drupal config state, but these did not fail because once config:import had run (via drush deploy), the configuration on disk matched the active configuration as far as Drupal was concerned.

If we are allowing folks to believe that configuration on disk is their source of truth and then modifying active configuration with an update hook, this seems like it will inevitably cause issues like this.

@markdorison
Copy link
Contributor

@apotek
Copy link
Contributor

apotek commented Jul 19, 2023

A module update done via one of our automated tools may have an update hook that changes configuration.

This possibility should almost always/always line up with a change to at least mymodule/config/mymodule.schema.yml and possibly the config/install/mymodule.settings.yml.

So if there were a change in those files between versions, we could anticipate that there is an update hook that modifies config. I've always hated that drupal's update hooks are so powerful (you can do anything you want in there) and that they therefore can easily cause these circular dependencies on deployment. It really breaks the config files as the source of truth. To my mind, changes to the config/schema of a module should introduce a different major version number, and the update to the module should therefore include some extra steps and not have an automated update path.

But, back to reality.... I think the best we can do here is try to predict that there will be an issue and alert the end user that they should probably resolve config changes manually before deploying, and interrupt the deployment.

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

No branches or pull requests

4 participants