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

Ngrok Modification #1048

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

Ngrok Modification #1048

wants to merge 8 commits into from

Conversation

sean-e-dietrich
Copy link
Member

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

@achekulaev achekulaev added this to To do in Docksal 1.13.0 via automation May 4, 2019
@achekulaev
Copy link
Member

Looks great to me. I would keep it open until 0.12.2 releases.

@lmakarov lmakarov self-requested a review October 3, 2019 19:29
Copy link
Member

@lmakarov lmakarov left a 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.

@lmakarov
Copy link
Member

lmakarov commented Oct 3, 2019

@sean-e-dietrich please fix tests.

@sean-e-dietrich
Copy link
Member Author

@lmakarov this has been updated and retested.

@lmakarov lmakarov removed this from To do in Docksal 1.13.0 Oct 7, 2019
@@ -8000,6 +8011,8 @@ case "$1" in
[[ "$USED_ALIAS" != "" ]] &&
[[ "$(pwd)" == "$PROJECT_ROOT" ]] &&
cd "$DOCROOT"
[[ "${NO_DRUSH_OPTIONS_URI}" != "" ]] &&
Copy link
Member

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?

@lmakarov
Copy link
Member

lmakarov commented Oct 7, 2019

@sean-e-dietrich please see my thoughts in #1046 (comment) regarding an alternative approach.

@achekulaev
Copy link
Member

Alternative approach is good for future but too many open questions. We should finish with this approach first.

@lmakarov
Copy link
Member

@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.

too many open questions

What are the open questions?

@achekulaev
Copy link
Member

achekulaev commented Oct 22, 2019

@lmakarov

Open questions of the alternate approach are:

  • How to get ngrok URL after I started sharing?
  • How to pass all additional ngrok params, that are currently passed as command line arguments? Can they be passed as ENV vars or does it need to be implemented additionally?
  • ngrok used to be a temporary share thingy, that turns off quickly by ctrl-c. With container approach it would have to be turned off to avoid sharing, which should be highlighted in the docs, because it might be a security implication
  • Someone needs to actually Test new approach
  • Someone needs to create new docs on using the new approach, that will include answers to all the questions above

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.

@sean-e-dietrich
Copy link
Member Author

sean-e-dietrich commented Oct 22, 2019

@achekulaev

How to get ngrok URL after I started sharing?

We will output the URL to the person using the Ngrok API

How to pass all additional ngrok params, that are currently passed as command line arguments Can they be passed as ENV vars or does it need to be implemented additionally?

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.

ngrok used to be a temporary share thingy, that turns off quickly by ctrl-c. With container approach it would have to be turned off to avoid sharing, which should be highlighted in the docs, because it might be a security implication

We can have little messages run every so often that share is still running so they are aware of it.

Someone needs to actually Test new approach

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.

Someone needs to create new docs on using the new approach, that will include answers to all the questions above

This is the same with everything we do.

Take a look here as a start and maybe this will help answer some questions

#1195

@achekulaev
Copy link
Member

achekulaev commented Oct 23, 2019

@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.

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.

Rework fin share as part of project
3 participants