-
-
Notifications
You must be signed in to change notification settings - Fork 177
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
Ngrok Modification #1048
base: develop
Are you sure you want to change the base?
Ngrok Modification #1048
Conversation
…riendlier dns names.
Looks great to me. I would keep it open until 0.12.2 releases. |
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.
@sean-e-dietrich please rebased onto develop
so be can see green tests.
@sean-e-dietrich please fix tests. |
@lmakarov this has been updated and retested. |
@@ -8000,6 +8011,8 @@ case "$1" in | |||
[[ "$USED_ALIAS" != "" ]] && | |||
[[ "$(pwd)" == "$PROJECT_ROOT" ]] && | |||
cd "$DOCROOT" | |||
[[ "${NO_DRUSH_OPTIONS_URI}" != "" ]] && |
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.
@sean-e-dietrich does this change belong here?
@sean-e-dietrich please see my thoughts in #1046 (comment) regarding an alternative approach. |
Alternative approach is good for future but too many open questions. We should finish with this approach first. |
@achekulaev The alternative approach I'm suggesting is doing the same thing, but with docker-compose, instead of bare docker. The end result is the same, but with less code and more reliable.
What are the open questions? |
Open questions of the alternate approach are:
I estimate the effort size to address these questions to be medium-to-large. Consequently to avoid pushing 1.13 release date further I believe it will be better to put it off till 1.14. |
We will output the URL to the person using the Ngrok API
We previously allowed for ENV vars and they were just mapping the existing container variables. Additionally, we allowed for the command line args to override the environment variables. This is still going to be the case.
We can have little messages run every so often that share is still running so they are aware of it.
We will need to adjust existing tests to use new approach but just a minor change to look at new service for the new container. Additionally, there will need to be more tests added.
This is the same with everything we do. Take a look here as a start and maybe this will help answer some questions |
@sean-e-dietrich I used "open questions" meaning matters that require time investment to get them ironed out, I did not mean questions literally. Of course, I understand or foresee the answers to those questions, but the gist of the comment is that there are those things and to avoid pushing 1.13 release date further I believed it would be better to put it off till 1.14. But if you guys feel like getting it done in 1.13 I do not want to hold the enthusiasm in any way. We just need it implemented with features discussed above, tested and documented, and while I am not in a position to help with implementation of this in scope of 1.13, I am totally ok to help reviewing a PR with those things from whoever does that, and can assist with testing it, if there is an agreement that we want to push for it in 1.13, and there is a will/time from someone else to actually do it. |
Modified Ngrok to connect to a docker project network.
This is also helpful when utilizing the configuration files to reference the hostname of the service.
Fixes #1046