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 over-eager project env replacement, for #4197 #4437

Merged
merged 1 commit into from Dec 8, 2022

Conversation

rfay
Copy link
Member

@rfay rfay commented Dec 7, 2022

The Problem/Issue/Bug:

See bug reported in

The application .env file replacement (for laravel, shopware, etc.) was matching partials on the key, so

For example, I have DYNAMODB_CONNECTION in my .env (from this package) and ddev thinks it is the same as DB_CONNECTION, and replaced my config with DYNAMODB_CONNECTION="mysql".

How this PR Solves The Problem:

  • Use a more sophisticated pattern for replacement
  • Add TestWriteProjectEnvFile that covers (hopefully) all the things

Manual Testing Instructions:

Try the env file suggested by @stasadev

Automated Testing Overview:

Added significant test in TestWriteProjectEnvFile

Related Issue Link(s):

@rfay rfay requested review from stasadev and tyler36 December 7, 2022 17:44
@github-actions
Copy link

github-actions bot commented Dec 7, 2022

Copy link
Member

@stasadev stasadev left a comment

Choose a reason for hiding this comment

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

It works, thank you!

Also, I noticed that:

  1. It writes the db config to .env even if you have omit_containers: [db]
  2. I use URL from additional_hostnames for APP_URL in Laravel, but it is always overridden with the primary URL (maybe you can use curl to check if APP_URL is already reachable, and do not override it).

@rfay
Copy link
Member Author

rfay commented Dec 7, 2022

Thanks so much @stasadev -

  1. Do you think it matters that it writes DB stuff if omit_containers[db]?
  2. I recommend using ddev config --disable-settings-management in your situation. Is that adequate for you?

@stasadev
Copy link
Member

stasadev commented Dec 7, 2022

  1. Just noticed it, does not matter to me.
  2. Yes, that's exactly what I want 👍

@rfay rfay merged commit fe70de9 into ddev:master Dec 8, 2022
@rfay rfay deleted the 20221207_env_vars_regex branch December 8, 2022 14:08
@rahaddon
Copy link

@rfay Hey, every time I perform a ddev restart on a laravel project which doesn't have standard ENV vars like DB_USER they are always being either automatically added or overwritten which results in having to undo the changes, is this the desired behaviour? Thanks

@rfay
Copy link
Member Author

rfay commented Jan 16, 2023

That is the expected behavior, but you can just turn it off with settings_management_disabled: true, see https://ddev.readthedocs.io/en/latest/users/usage/cms-settings/

But I'd love to have you explain in more detail why your laravel project doesn't use those so I could understand better.

@rahaddon
Copy link

rahaddon commented Jan 17, 2023

Thank you for getting back to me, does settings_management_disabled: true disable anything else or it is just related to auto config overwrites in the .env file based on the project type?

But I'd love to have you explain in more detail why your laravel project doesn't use those so I could understand better.

We are using a remote staging database environment which is shared between development team members so the credentials are different to those defaults which are auto-populated by DDEV. Each team member has their own unique user credentials to access the database.

@rfay
Copy link
Member Author

rfay commented Jan 17, 2023

I think settings_management_disabled: true will do what you want.

I do recommend using ddev pull to get the database rather than this, as it will prevent unexpected results of using a shared db. Or just sharing a db dump. But I know you've already thought this through.

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

3 participants