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

Lets us specify init flags #483

Merged
merged 1 commit into from Mar 5, 2018
Merged

Conversation

CMajeri
Copy link

@CMajeri CMajeri commented Mar 1, 2018

As discussed in issue #481, this PR makes the use of --init flags when using --docker-run more reliable by detecting the use.

Before landing

  • Update docs/reference/changelog.md
  • Link to applicable GH issues in the changelog

@ark3
Copy link
Contributor

ark3 commented Mar 1, 2018

Your implementation (in container.py) is more complicated than I expected. Why modify parse_docker_args(...) at all? It seems all we need is an additional test in the if statement on (original) line 163. If docker_args does not already have an --init, then add one.

Sadly there is no straightforward approach to testing if you make the change that way... But that's a symptom of the poor organization of the code as a whole.

@CMajeri
Copy link
Author

CMajeri commented Mar 1, 2018

I thought about doing that until you mentioned the --publish. I figured it would make more sense to treat all "specfic" parameters in one place (and when/if that becomes too big, move them and change the way they're treated).

I'm okay with moving the if inside the run command, but that makes it untestable as you've said.
And I find it makes the code slightly muddier.

@CMajeri
Copy link
Author

CMajeri commented Mar 5, 2018

I refactored it as you asked, it's much simpler and smaller, but unfortunately it can't be tested that way.
I tried it out by printing the docker_command, and it seems to do what is expected.

@ark3 ark3 merged commit 435630d into telepresenceio:master Mar 5, 2018
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.

None yet

2 participants