-
-
Notifications
You must be signed in to change notification settings - Fork 581
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: output the VIRTUAL_HOST if specified in environment variables of a service #6136
feat: output the VIRTUAL_HOST if specified in environment variables of a service #6136
Conversation
Download the artifacts for this pull request:
See Testing a PR. |
I actually see a better place for this - will update the PR. |
Buildkite apparently failed for testnotddevapp - is there a way to quickly check that locally? |
It's likely a transient failure. If you PM me your email I'll give you privileges on buildkite and you can restart tests. |
Yes, appears that it was just a transient failure. |
ac34de0
to
cf657ac
Compare
Rebased, mostly just to run a new test runner. |
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, ddev describe
looks good, I just have a small suggestion.
Yeah that makes sense, accepted the change. |
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.
Thank you!
I'm sorry we didn't get this into v1.23.1, we were trying to make it as minimally disruptive as possible. It should get into v1.23.2. |
…les of a service.
Co-authored-by: Stanislav Zhuk <stasadev@gmail.com>
427aaaf
to
a5e0398
Compare
Rebased. |
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, looks great to me. Manually tested in a number of situations.
Maybe a followup docs PR explaining how to use VIRTUAL_HOST as used here would be a good thing.
I used a .ddev/docker-compose.solr_extra.yaml with
services:
solr:
environment:
- VIRTUAL_HOST=mysolr.${DDEV_HOSTNAME}
And a config.solr.yaml with:
additional_hostnames: [mysolr.d10]
…f a service (ddev#6136) [skip ci] Co-authored-by: Stanislav Zhuk <stasadev@gmail.com>
The Issue
Right now,
ddev describe
outputs most of the information one needs to access the various services in a DDEV project. There are cases however where it neglects to output customVIRTUAL_HOST
values fromenvironment
sections of a Docker Compose service. These values can allow for vanity domains of a service. This could/would be common with add-on services like Solr, NodeJS applications or similar.How This PR Solves The Issue
This PR modifies describe.go to try and pull in the information of the
environment
section of a service.Manual Testing Instructions
VIRTUAL_HOST=site1.$DDEV_HOSTNAME
"describe
command.Before:
Note the NodeJS and Solr services.
After:
Note that the NodeJS and Solr services now show their correct
VIRTUAL_HOST
values.These services are now available in the browser at those hostnames. Otherwise, it can appear to be unclear that this is possible to end users. There are cases where users may prefer to access services at custom hostnames instead of default + port and this will add the output to
ddev describe
.I could not figure out however how to prepend http:// or https:// to these values.
Automated Testing Overview
These changes do not modify the structure of the describe output, therefore the current
TestCmdDescribe
needs no change.