Implement DdevGlobalConfig initialization in one place #5007
Conversation
rfay
force-pushed
the
20230621_globalconfig_new
branch
from
42063cc
to
f060fff
Compare
|
Download the artifacts for this pull request:
See Testing a PR |
In ddev#4995 we had to do initialization of RouterHTTPPort deep in unrelated code. Since it's now done in globalconfig initialization we don't have to do that now
rfay
force-pushed
the
20230621_globalconfig_new
branch
from
b968bcf
to
b7b5f17
Compare
| // Remove some items that are defaults | ||
| if cfgCopy.RequiredDockerComposeVersion == RequiredDockerComposeVersionDefault { | ||
| cfgCopy.RequiredDockerComposeVersion = "" | ||
| } |
There was a problem hiding this comment.
I'd avoid this completely. Instead use a getter function which returns a sane default if the config value is empty. Similar you did with the http ports above.
There was a problem hiding this comment.
Oh I see the function bellow which is used in the code. There you also could check for a value in the global config or use the default if empty.
There was a problem hiding this comment.
Instead of this code, better fix it on loading.
There was a problem hiding this comment.
No, the problem here is on writing. We just want to hide this value from people. We need it on reading (normally the default). But note upcoming
There was a problem hiding this comment.
Yes, that's clear, the default value should never be written to the file. But still I think validation and removal should better be done after reading and a getter function should take care to return the default value. Anyway, that's not a stopper, go ahead.
| TableStyle: "default", | ||
| RouterHTTPPort: "80", | ||
| RouterHTTPSPort: "443", | ||
| LastStartedVersion: "v0.0", | ||
| NoBindMounts: nodeps.NoBindMountsDefault, | ||
| UseTraefik: nodeps.UseTraefikDefault, |
There was a problem hiding this comment.
I see that these should be constants, I don't want to do it right now but will try to fix that up in the traefik PR.
The Issue
@gilbertsoft pointed out elsewhere that globalconfig.DdevGlobalConfig initialization should be encapsulated in a New() function.
How This PR Solves The Issue
Do it and work it all out.
This one ends up being a pre-requisite for some other PRs, especially