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

fix: ddev config should not change unspecified options, fixes #5882 #5887

Merged
merged 1 commit into from Feb 28, 2024

Conversation

stasadev
Copy link
Member

@stasadev stasadev commented Feb 22, 2024

The Issue

The bug introduced in

The expected behavior for ddev config in the various situations:

  • config.yaml specifies a set of configs but there's no code to match it (the bug here). (This can be more than just project type I think, docroot as well).

    Running ddev config on existing code should not change existing config options that are not specified.
    For example, when you run ddev config --web-environment-add FOO=bar on a project with the latest config (updating the outdated configuration is mentioned later), it should only change the web_environment option, and nothing else. This is covered in this PR in TestConfigSetValues - this test checks a lot of configuration options, so I think we are safe here.

  • config.yaml specifies a set of configs but the code found by DDEV discovers other things (configured for drupal10, finds magento2)

    If the auto-detected project type is different from the one specified by the user, an additional run of ddev config should just show a warning about it, and not add project-specific files of the auto-detected project type. The user's preference should be respected, and if they choose drupal10, the drupal10 settings file (i.e. settings.ddev.php) should be added even if magento2 is automatically detected. This is true until disable_settings_management: true, when all settings are ignored. The project type change is covered in this PR in TestConfigSetValues - if the type is auto changed, we will know about it immediately.

  • Initial configuration autodetect, which is good, but of course doesn't work when there's no code.

    ddev config --auto should automatically detect the project type and set other options depending on the project type, but this only applies to the first project config run, all other ddev config --auto should respect the user's decision and only remove deprecated (i.e. outdated) or non-existent config options. This does not require testing, as yaml.Marshal() is used for app.WriteConfig() and all junk configs will always be removed. And auto-detection itself is covered by several tests with Detection in the name.

  • config.yaml merges additional config.*.yaml, and you can override config instead of merging with override_config: true

    This is explained in the docs and covered with TestConfigOverrideDetection.

How This PR Solves The Issue

Fixes an issue with overriding the project type when it should not have been changed.
Checks for this bug with a test.

Manual Testing Instructions

  1. ddev config --auto && ddev config --project-type drupal10 && ddev config --auto
  2. check that project type is drupal10, not php.

Automated Testing Overview

Related Issue Link(s)

Release/Deployment Notes

@stasadev stasadev requested a review from a team as a code owner February 22, 2024 21:12
Copy link

github-actions bot commented Feb 22, 2024

@stasadev stasadev force-pushed the 20240222_stasadev_fix_config_override branch from b63c6fc to b03fd4a Compare February 22, 2024 21:18
@rfay
Copy link
Member

rfay commented Feb 23, 2024

Wow, you got a clean colima/github test!

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

Thanks for adding test coverage. Please update the OP to state the full expected behavior in the various situations:

  • Config.yaml specifies a set of configs but there's no code to match it (the bug here). (This can be more than just project type I think, docroot as well).
  • Config.yaml specifies a set of configs but the code found by DDEV discovers other things (configured for drupal10, finds magento2)
  • Initial configuration autodetect, which is good, but of course doesn't work when there's no code.
  • There may be other permutations, but this basic problem has dogged people from time to time.

Part of the issue is that during the lifetime of DDEV people have moved from initial setup being a git checkout of existing code (with docroot, etc) to where they often start a project with no code at all and a ddev composer create. So the code changes after the configuration is done. And of course, the configuration might not have been right.

@stasadev stasadev force-pushed the 20240222_stasadev_fix_config_override branch from b03fd4a to e461701 Compare February 23, 2024 18:28
@stasadev
Copy link
Member Author

Rebased and updated the OP with the expected behavior.
Looking deeper, I saw that different situations are already covered in other tests, and I simply covered the missing, untested part.

@jpoesen
Copy link

jpoesen commented Feb 24, 2024

👍

Using v1.22.7-48-ge461701c1, this use case now seems to work perfectly on Ubuntu:

  • create and change into new project dir
  • ddev config, choosing drupal10 project type
  • ddev config --http-port=8123
    --> Before this ddev version, project type was reset to 'php'.
    --> Using this ddev version, project type remained 'drupal10` (success!)

Thanks!

@rfay rfay force-pushed the 20240222_stasadev_fix_config_override branch from e461701 to af3d18a Compare February 25, 2024 20:39
@rfay
Copy link
Member

rfay commented Feb 25, 2024

Rebased

@rfay rfay force-pushed the 20240222_stasadev_fix_config_override branch from af3d18a to 32bda6a Compare February 28, 2024 01:14
@rfay
Copy link
Member

rfay commented Feb 28, 2024

Rebased.

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

Thanks, this is great.

I also manually tested with lots and lots of permutations, and it all behaved as expected.

@rfay rfay merged commit f6474f2 into ddev:master Feb 28, 2024
17 checks passed
@stasadev stasadev deleted the 20240222_stasadev_fix_config_override branch February 28, 2024 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants