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: use dynamic ports for router and mailpit, fixes #23 #30

Merged
merged 3 commits into from
Jan 21, 2025

Conversation

zeshanziya
Copy link
Contributor

@zeshanziya zeshanziya commented Jan 17, 2025

The Issue

Issue #23

How This PR Solves The Issue

Updated the docker-compose.varnish.yaml file to use environment variables (DDEV_ROUTER_HTTPS_PORT and DDEV_ROUTER_HTTP_PORT) for dynamic port configuration.
This change ensures compatibility with custom DDEV port settings and resolves issues when default ports are unavailable

Manual Testing Instructions

ddev config --router-http-port 8080 --router-https-port 8443 --mailpit-http-port 18025 --mailpit-https-port 18026
ddev add-on get https://github.com/zeshanziya/ddev-varnish/tarball/dynamic-router-ports
ddev restart

And look inside:

cat .ddev/.ddev-docker-compose-full.yaml

Automated Testing Overview

Related Issue Link(s)

Release/Deployment Notes

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.

Thank you @zeshanziya !

Yes, that's the right approach, and while we're here, let's update the Mailpit ports too.

DDEV template for the web container has the same env variables https://github.com/ddev/ddev/blob/db18c34739faecd597fbdbb36c6ad743a82e8c45/pkg/ddevapp/app_compose_template.yaml#L203-L208

docker-compose.varnish.yaml Outdated Show resolved Hide resolved
@rfay
Copy link
Member

rfay commented Jan 17, 2025

This PR would be massively better if it had a test for this situation...

@stasadev
Copy link
Member

stasadev commented Jan 18, 2025

A new test should be added to tests/test.bats

Basically, it should be a copy-paste "install from directory" with the addition of:

@test "install from directory with nonstandard port" {
  set -eu -o pipefail
  cd ${TESTDIR}
  ddev config --router-http-port 8080 --router-https-port 8443 --mailpit-http-port 18025 --mailpit-https-port 18026
  ...
  # change ports here in curl calls
}

And the check should be done for other ports in the test itself instead of the standard ones.

@zeshanziya
Copy link
Contributor Author

@stasadev Thank you for the guidance. I have updated the Mailpit ports as suggested and added a test case for non-standard ports. Please review.

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.

Looks great and the new test works, thank you!

@stasadev stasadev changed the title Use dynamic DDEV router ports fix: use dynamic ports for router and mailpit, fixes #23 Jan 21, 2025
@stasadev stasadev merged commit 2c492ce into ddev:main Jan 21, 2025
2 checks passed
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.

3 participants