fix: make extra docker-compose always have correct primary hostname, fixes #21 #25

FlorentTorregrosa · 2023-12-12T09:22:28Z

The Issue

When changing a project name in .ddev/config.yaml because a git repository is used as a starterkit for other projects, it is easy to forgot that .ddev/docker-compose.varnish-extras.yaml has a VIRTUAL_HOST hardcoded with the starterkit value.

How This PR Solves The Issue

Replacing the previously generated hostname with VIRTUAL_HOST=novarnish.${DDEV_PROJECT}.${DDEV_TLD},novarnish.extra.${DDEV_TLD},... avoids to have to change that for every project.

Manual Testing Instructions

This allows for example to have Mailpit to work automatically without any port change.

mkdir varnish-test && cd varnish-test
ddev config --additional-fqdns="hello.test" --additional-hostnames "extra1,extra2,more.extra"
ddev add-on get https://github.com/FlorentTorregrosa/ddev-varnish/tarball/21

cat .ddev/docker-compose.varnish_extras.yaml
#  - VIRTUAL_HOST=novarnish.${DDEV_PROJECT}.${DDEV_TLD},novarnish.extra1.${DDEV_TLD},novarnish.extra2.${DDEV_TLD},novarnish.hello.test,novarnish.more.extra.${DDEV_TLD}

ddev start
ddev exec echo '$VIRTUAL_HOST'
# novarnish.varnish-test.ddev.site,novarnish.extra1.ddev.site,novarnish.extra2.ddev.site,novarnish.hello.test,novarnish.more.extra.ddev.site

Automated Testing Overview

Release/Deployment Notes

stasadev

Hi @FlorentTorregrosa,

Although this approach may work when added to yaml: VIRTUAL_HOST=novarnish.$DDEV_HOSTNAME,
it doesn't work for the install script because $DDEV_HOSTNAME is expanded to its value.

I tried to find a way around this for Go Templates Parse, but no luck.

And on the other hand, $DDEV_HOSTNAME can contain not only one domain, if additional_hostnames is not empty, then $DDEV_HOSTNAME should be set to ${DDEV_HOSTNAME%%,*}, but in this case it doesn't work in yaml.

FlorentTorregrosa · 2023-12-13T17:19:39Z

Hi @stasadev,

Thanks for the feedback.

Ok, I am new to ddev, so I don't think I will be able to move this PR more forward.

If at least it will help people and move the issue forward, I will stay with that :)

stasadev

When you have additional hostnames:

ddev config --additional-hostnames one,two

The contents of docker-compose.varnish-extras.yaml with this PR will be:

services:
  web:
    environment:
    - VIRTUAL_HOST=novarnish.$DDEV_HOSTNAME,novarnish.one.ddev.site,novarnish.two.ddev.site

And VIRTUAL_HOST will be expanded to novarnish.sitename.ddev.site,one.ddev.site,two.ddev.site,novarnish.one.ddev.site,novarnish.two.ddev.site, so it will have one.ddev.site,two.ddev.site without novarnish. prefix.

I don't know what the implications are for these values in VIRTUAL_HOST, just stating what I see.

Hmm, it doesn't pass the tests, so I guess this change can't work well with additional hostnames.

stasadev · 2024-07-04T17:46:09Z

install.yaml

@@ -29,7 +29,7 @@ post_install_actions:
    {{ $project_tld := "ddev.site" }}
    {{ if .DdevGlobalConfig.project_tld }}{{ $project_tld = .DdevGlobalConfig.project_tld }}{{ end }}
    {{ if .DdevProjectConfig.project_tld }}{{ $project_tld = .DdevProjectConfig.project_tld }} {{ end }}
-    {{ $novarnish_hostnames := print "novarnish." .DdevProjectConfig.name "." $project_tld  }}
+    {{ $novarnish_hostnames := print "novarnish.$DDEV_HOSTNAME" }}


I found a way to escape $ with \\$:

Suggested change

{{ $novarnish_hostnames := print "novarnish.$DDEV_HOSTNAME" }}

{{ $novarnish_hostnames := print "novarnish.\\$DDEV_HOSTNAME" }}

rfay · 2024-07-05T12:23:20Z

install.yaml

@@ -29,7 +29,7 @@ post_install_actions:
    {{ $project_tld := "ddev.site" }}
    {{ if .DdevGlobalConfig.project_tld }}{{ $project_tld = .DdevGlobalConfig.project_tld }}{{ end }}
    {{ if .DdevProjectConfig.project_tld }}{{ $project_tld = .DdevProjectConfig.project_tld }} {{ end }}
-    {{ $novarnish_hostnames := print "novarnish." .DdevProjectConfig.name "." $project_tld  }}
+    {{ $novarnish_hostnames := print "novarnish.\\$DDEV_HOSTNAME" }}


An option here is to use sprig's env function I think, https://masterminds.github.io/sprig/os.html

We don't want to expand it (PR is about making it dynamic).

The current behavior (not in this PR) already expands all hostnames, but @FlorentTorregrosa wants it to work even when you change the project name.

The first stopper was that go parser expanded $DDEV_HOSTNAME to it's value. I managed to prevent the expansion with \\$DDEV_HOSTNAME, so the resulting yaml looks like VIRTUAL_HOST=novarnish.$DDEV_HOSTNAME.

Now the problem is that the tests fail, because novarnish. is added only to the first hostname in novarnish.$DDEV_HOSTNAME.

But we can't do any bash manipulation on the resulting yaml file.

Sadly, I don't see a way to make it work.

stasadev

I found a way how to make it work. See manual instructions.

rfay

So great, almost done, thanks!

rfay · 2024-12-24T15:55:52Z

install.yaml

    services:
      web:
        environment:
-        - VIRTUAL_HOST={{ $novarnish_hostnames }}
+        {{- $novarnish_hostnames := splitList "," (env "DDEV_HOSTNAME") }}
+        - VIRTUAL_HOST={{ range $i, $n := $novarnish_hostnames }}novarnish.{{ replace (env "DDEV_TLD") "\\${DDEV_TLD}" (replace (env "DDEV_PROJECT") "\\${DDEV_PROJECT}" $n) }}{{ if lt (add1 $i) (len $novarnish_hostnames) }},{{ end }}{{ end }}


Please add a comment here to explain what this is doing and why. It looks great and should completely solve this problem.

The README needs to explain that if additional_hostnames or additional_fqdns changes, the ddev add-on get needs to be re-run.

It will need a new release as well.

The comment was already here above the services:

I'm not very clear on how it works, but I've added more explanations there.

rfay

Thanks. Even though I do occasionally start understanding Go templates I still hate them :)

FlorentTorregrosa · 2024-12-28T14:13:41Z

Hi,

Thanks to have pushed this PR forward and for the merge!

FlorentTorregrosa mentioned this pull request Dec 12, 2023

Access to MailPit (was MailHog) redirects to novarnish.[hostname] #21

Closed

stasadev requested changes Dec 13, 2023

View reviewed changes

stasadev reviewed Jul 4, 2024

View reviewed changes

stasadev changed the title ~~Fix #21: Fix extra docker-compose to always have correct hostname.~~ fix: make extra docker-compose to always have correct primary hostname, fixes #21 Jul 4, 2024

stasadev force-pushed the 21 branch from 71612cb to e44210f Compare July 4, 2024 18:02

rfay reviewed Jul 5, 2024

View reviewed changes

Make extra docker-compose work with env variables

b63baf9

stasadev force-pushed the 21 branch from e44210f to b63baf9 Compare December 20, 2024 14:33

stasadev approved these changes Dec 20, 2024

View reviewed changes

stasadev requested a review from rfay December 20, 2024 14:41

stasadev mentioned this pull request Dec 20, 2024

fix: add support for multiple hostnames tyler36/ddev-cypress#54

Merged

rfay changed the title ~~fix: make extra docker-compose to always have correct primary hostname, fixes #21~~ fix: make extra docker-compose always have correct primary hostname, fixes #21 Dec 24, 2024

rfay requested changes Dec 24, 2024

View reviewed changes

Update descriptions, add a note

02ec998

stasadev requested a review from rfay December 26, 2024 22:05

rfay approved these changes Dec 26, 2024

View reviewed changes

stasadev merged commit 053aa16 into ddev:main Dec 26, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: make extra docker-compose always have correct primary hostname, fixes #21 #25

fix: make extra docker-compose always have correct primary hostname, fixes #21 #25

FlorentTorregrosa commented Dec 12, 2023 •

edited by stasadev

Loading

stasadev left a comment

FlorentTorregrosa commented Dec 13, 2023

stasadev left a comment •

edited

Loading

stasadev Jul 4, 2024

rfay Jul 5, 2024

stasadev Jul 5, 2024

stasadev left a comment

rfay left a comment

rfay Dec 24, 2024

stasadev Dec 26, 2024

rfay left a comment

FlorentTorregrosa commented Dec 28, 2024

	{{ $novarnish_hostnames := print "novarnish.$DDEV_HOSTNAME" }}
	{{ $novarnish_hostnames := print "novarnish.\\$DDEV_HOSTNAME" }}

fix: make extra docker-compose always have correct primary hostname, fixes #21 #25

fix: make extra docker-compose always have correct primary hostname, fixes #21 #25

Conversation

FlorentTorregrosa commented Dec 12, 2023 • edited by stasadev Loading

The Issue

How This PR Solves The Issue

Manual Testing Instructions

Automated Testing Overview

Related Issue Link(s)

Release/Deployment Notes

stasadev left a comment

Choose a reason for hiding this comment

FlorentTorregrosa commented Dec 13, 2023

stasadev left a comment • edited Loading

Choose a reason for hiding this comment

stasadev Jul 4, 2024

Choose a reason for hiding this comment

rfay Jul 5, 2024

Choose a reason for hiding this comment

stasadev Jul 5, 2024

Choose a reason for hiding this comment

stasadev left a comment

Choose a reason for hiding this comment

rfay left a comment

Choose a reason for hiding this comment

rfay Dec 24, 2024

Choose a reason for hiding this comment

stasadev Dec 26, 2024

Choose a reason for hiding this comment

rfay left a comment

Choose a reason for hiding this comment

FlorentTorregrosa commented Dec 28, 2024

FlorentTorregrosa commented Dec 12, 2023 •

edited by stasadev

Loading

stasadev left a comment •

edited

Loading