Config validator fails to validate on non-essential differences

[apotek]

Description

The config validation check that we run post config:import can in some cases report a configuration mismatch due Drupal not always matching what is in yaml (and vice a versa).

The example I am experiencing right now is that the migration configuration files in JFK have comments in them, as well as some pretty advanced yaml merging functionality that Drupal tends to resolve and throw away on import.

So, in tugboat, I get these errors:

robo --ansi 'validate:drupal-config'
.... (long list of mismatched config entries)
[error] in task Usher\Robo\Plugin\Commands\ValidateConfigCommands

Drupal database configuration does not match the tracked file system configuration.
Script robo --ansi handling the robo event returned with error code 1

The deployment does seem to complete successfully (Tugboat appears to think it was successful) so it is not breaking anything, but it's not great to see red error text at the bottom of the build log.

Can we either: 1) Drop this from error level to warning level (yellow) since it is not actually failing the build? OR 2) Provide a way to report the diff informationally?

I can't think of a way to do the config check/validation in a way that won't flag these differences, so maybe we just provide a lower level of reporting by default, or possibly also a way to set a lower level via a flag, something like so:

       online:
        # Validate that the code and database configuration match.
        - 'composer robo validate:drupal-config --set-pr-status --mismatch-info|mismatch-warn|mismatch-panic|mismatch-error'
        

[apotek]

Hmmm. I wonder if this code is actually working as intended:

            if (!is_array($configJson) || $configJson !== []) {
                $this->say($drushOutput);
                if ($setPrStatus) {
                    $this->setGitHubStatusError(
                        gitHubCheckName: self::GITHUB_STATUS_CHECK_NAME,
                        gitHubCheckDescription: 'Drupal config validation failed!'
                    );
                }
                throw new TaskException(
                    $this,
                    'Drupal database configuration does not match the tracked file system configuration.'
                );
            }

Because Tugboat is still saying the build was successful, even though the build log has the error and the build stops.

Looking at this more in-depth, I see that the options are set right, so perhaps setGitHubStatusError is not working 🤔

[markdorison]

Because Tugboat is still saying the build was successful

@apotek We have typically set this up as a check that is separate from the Tugboat build itself. So the Tugboat build is successful and shows as such in the PR status and then this check runs and sets its own status.

[adamzimmermann]

@apotek and I chatted about this some earlier today.

The summary is:

• I can really empathize with the desire to leave comments in yml files, as I love code comments as much if not more than the next person.
• However, Drupal does not play nicely with them and we can't differentiate between a diff from comments or a diff that could cause a real issue or just confuse developers with an extra diff.
• Thus the impact to developer experience needs to trump the desire to leave in comments in my opinion.

tl;dr this is a feature not a bug.

However.... there are times were we want to allow a diff potentially. There are two ways we could do this:

1. Allow usher to be configured to report a warning and not an error to the GitHub check. However, it seems that this is not possible (https://docs.github.com/en/rest/commits/statuses?apiVersion=2022-11-28).
2. Simply tell our users to configure if the check is required in their GitHub required checks configuration in the branch protection rules.

Given that the former option seems not feasible, I think things are pretty straightforward since we only have one options. 😂

@apotek thoughts?

[apotek]

1. Allow usher to be configured to report a warning and not an error to the GitHub check. However, it seems that this is not possible (https://docs.github.com/en/rest/commits/statuses?apiVersion=2022-11-28).

It's not possible to send a warning or info status, but we could opt to send a success status and put warning text in the description.

2. Simply tell our users to configure if the check is required in their GitHub required checks configuration in the branch protection rules.

This works too, and is, it seems, the supported path. I just prefer a "temporary" flag for overriding the build error when things are in a known unstable state as opposed to reconfiguring the repo (which one might likely forget to ever switch back)

Nevertheless, let's do what you suggest. We'll make it supported that one can override the error.

[adamzimmermann]

It's not possible to send a warning or info status, but we could opt to send a success status and put warning text in the description.

You are correct. I didn't think of that. I could see that being glossed right over and it would be more complex to configure too IMO.

---

This works too, and is, it seems, the supported path. I just prefer a "temporary" flag for overriding the build error when things are in a known unstable state as opposed to reconfiguring the repo (which one might likely forget to ever switch back)

I share your concern with forgotten "temporary" flags, but I could say the same thing about changing the configuration of the check. One change is done via GitHub admin and one requires a code change. So I guess this is a case of needing to pick your poison. 🙁

---

whenever i do a drush cex i get a ton of migrate_plus.migration.orange_dam* config files that it wants to overwrite. the values in the db and in the files are identical, but the files have comments, which drupal dislikes. are there any recommendations for how i can make this less of a bother

This was just shared this with us this morning, which only reinforces the importance of the developer experience this check seeks to address.