-
-
Notifications
You must be signed in to change notification settings - Fork 602
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
feat: accept port in ddev launch
, for #5525
#6024
Conversation
Download the artifacts for this pull request:
See Testing a PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested this on gitpod and worked as expected.
cmd/ddev/cmd/commands_test.go
Outdated
@@ -347,6 +348,7 @@ func TestLaunchCommand(t *testing.T) { | |||
"nested/path": app.GetPrimaryURL() + "/nested/path", | |||
"/path-with-slash": app.GetPrimaryURL() + "/path-with-slash", | |||
app.GetPrimaryURL() + "/full-path": app.GetPrimaryURL() + "/full-path", | |||
":3000": primaryURLWithoutPort + ":3000", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add another test for ":3000/test"?
":3000": primaryURLWithoutPort + ":3000", | |
":3000": primaryURLWithoutPort + ":3000", | |
":3000/test": primaryURLWithoutPort + ":3000/test", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised it worked on gitpod, which is very picky about these things, and doesn't use ddev-router
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As gitpod is special in its port handling, I'll be more concrete:
ddev launch
: opens https://8080-stasadev-ddev-ajl5zssk1i2.ws-eu110.gitpod.io (made sense to me)ddev launch :3000
: opens https://3000-stasadev-ddev-ajl5zssk1i2.ws-eu110.gitpod.io, not found (made sense to me)ddev launch :3000/test
: opens https://3000-stasadev-ddev-ajl5zssk1i2.ws-eu110.gitpod.io/test, not found (made sense to me)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also:
gitpod /workspace/d10simple (main) $ DDEV_DEBUG=true ddev launch :3000
FULLURL http://127.0.0.1:3000
gitpod /workspace/d10simple (main) $ DDEV_DEBUG=true ddev launch :3000/test
FULLURL http://127.0.0.1:3000/test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I added more tests.
elif [[ $1 =~ ^: ]]; then | ||
# specific port | ||
FULLURL="${FULLURL%:[0-9]*}${1}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if the port is for http
but the FULLURL
is https
? Or the other way around?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The protocol is not changed on ddev (might be redirected by e.g. nginx), so you can end on
400 Bad Request
The plain HTTP request was sent to HTTPS port
nginx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I thought was probably the case.
It feels like this PR is missing a trick there. With the original PR (and the alternative I suggested) checking HTTP vs HTTPS and setting the correct port is managed, so we don't get into this situation.
Right now ddev launch
has logic to check whether it should be using HTTP or HTTPS - but that logic isn't going to be leveraged here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at ddev adminer for example, there's just as much code figuring out what the URL should be (http vs https etc) as there is launching it. Ideally that could all be abstracted away together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if the port is for http but the FULLURL is https? Or the other way around?
It feels like this PR is missing a trick there.
Good point, thank you. I added detection for http/https.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at ddev adminer for example, there's just as much code figuring out what the URL should be (http vs https etc) as there is launching it. Ideally that could all be abstracted away together.
The port for adminer can be shortened right now to (assuming someone is using DDEV v1.23.0-alpha1):
DDEV_ADMINER_PORT=9100
DDEV_ADMINER_HTTPS_PORT=9101
if [ ${DDEV_PRIMARY_URL%://*} = "https" ]; then
ddev launch $DDEV_PRIMARY_URL:$DDEV_ADMINER_HTTPS_PORT
else
ddev launch $DDEV_PRIMARY_URL::$DDEV_ADMINER_PORT
fi
or after this PR:
DDEV_ADMINER_PORT=9100
DDEV_ADMINER_HTTPS_PORT=9101
if [ ${DDEV_PRIMARY_URL%://*} = "https" ]; then
ddev launch :$DDEV_ADMINER_HTTPS_PORT
else
ddev launch :$DDEV_ADMINER_PORT
fi
But I'm not going to change it in the near future, because people can use the old DDEV with the new add-on version. And we have no restrictions on this. Ideally, add-ons should work like composer packages and pull only compatible add-on version, but at the moment that's probably too much work.
Maybe I'm missing something, but this works in $ ddev -v
ddev version v1.22.7
$ DDEV_DEBUG=true ddev launch :8026
FULLURL https://tempest-l11-demo.ddev.site:8026
$ DDEV_DEBUG=true ddev launch $DDEV_PRIMARY_URL:3000
FULLURL https://tempest-l11-demo.ddev.site:3000 |
Please ignore my previous comment. I updated to HEAD, played around and have now $ ddev -v
ddev version v1.22.7
$ DDEV_DEBUG=true ddev launch :8026
FULLURL https://tempest-l11-demo.ddev.site/:8026 Appologies for the false alarm. |
I had a similar situation, the port worked for me at some point, and I added it to the documentation. But when I tried it later, it turned out that it didn't work at all. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on WSL2 (Ubuntu 22.04.4)
Current HEAD:
$ ddev -v
ddev version v1.23.0-alpha1
$ DDEV_DEBUG=true ddev launch :3000
FULLURL https://lara11-base-demo.ddev.site/:3000
This PR:
$ ddev -v
ddev version v1.23.0-alpha1-4-g71b07fb61
$ DDEV_DEBUG=true ddev launch :3000
FULLURL https://lara11-base-demo.ddev.site:3000
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the great work on this, and especially adding the extra cases to the test. It tests great manually and code is great. Thanks!
The Issue
I added an example in #5986 to run
ddev launch
with a specific port:But this never actually worked:
$DDEV_PRIMARY_URL
is not expanded because it is not available on the host side and$DDEV_PRIMARY_URL:3000
and:3000
are the same thing.ddev launch $DDEV_PRIMARY_URL:3000
only works if it is run in another DDEV custom script.How This PR Solves The Issue
Adds actual support to launch ports.
Manual Testing Instructions
See a tip for HTTP to HTTPS redirect at https://ddev--6024.org.readthedocs.build/en/6024/users/usage/commands/#launch
Automated Testing Overview
Related Issue Link(s)
Release/Deployment Notes