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: global project_tld should default to 'ddev.site', not empty, fixes #4290 #5632

Merged
merged 4 commits into from Dec 25, 2023

Conversation

rfay
Copy link
Member

@rfay rfay commented Dec 14, 2023

The Issue

@rpkoller pointed out that the global_config.yaml would be more useful if it just specified ddev.site as the project_tld.

How This PR Solves The Issue

Make it explicit in the config.

Manual Testing Instructions

  • Test to make sure changing global project_tld still works fine
  • Test to make sure default config with ddev.site works fine. Automated tests ought to do this just fine.

Copy link

github-actions bot commented Dec 14, 2023

@rfay rfay marked this pull request as ready for review December 15, 2023 20:08
@rfay rfay requested a review from a team as a code owner December 15, 2023 20:08
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.

Noticed a bug, it's not even related to the changes made in this PR (reproducible on the stable DDEV), but it probably fits here:

When you set the global TLD to test and the project TLD to ddev.site, the project config is not set, it seems that there is some hardcode in the code for nodeps.DdevDefaultTLD:

ddev config global --project-tld test
ddev config --project-tld ddev.site

On the other hand, this override works if I set it manually: project_tld: ddev.site in .ddev/config.yaml. But it is lost when I run ddev config --auto or ddev config --project-tld ddev.site.

@rfay
Copy link
Member Author

rfay commented Dec 19, 2023

OK with me for you to fix that here.

@stasadev
Copy link
Member

I made a change in the app.WriteConfig(), will see how the tests go.

@stasadev
Copy link
Member

It should be fine, I tested it again and didn't find any flaws.

@stasadev stasadev merged commit e160c18 into ddev:master Dec 25, 2023
18 checks passed
@stasadev stasadev deleted the 20231214_default_ddev_site branch December 25, 2023 18:19
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

2 participants