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

Make beaker-docker work in a docker container #2

Merged
merged 4 commits into from
Aug 9, 2017

Conversation

hedinasr
Copy link
Contributor

@hedinasr hedinasr commented Aug 7, 2017

Hey ! Please review and let me know what you think!

The solution is pretty simple: I check if I'm currently in
a container and then I choose the `Gateway` ip.
@puppetlabs-jenkins
Copy link
Contributor

Can one of the admins verify this patch?

@rishijavia
Copy link
Contributor

@Ananasr, we follow beaker's contributing guideline for all our hypervisor libraries.

Would you mind filing a ticket and also describing the problem in detail for us to review this patch?

@rishijavia
Copy link
Contributor

Also what do you think about fixing the port mapping in the first place itself? I made this change on my fork.

Docker changes the port mapping for port 22 every time the container restarts. It would be easier for us to just hardcode this mapping to a random port during the creation of the container so that it would not change on restart and it'd be easy for us to SSH.

@hedinasr
Copy link
Contributor Author

hedinasr commented Aug 7, 2017

Can you show me a use case where the current port mapping is a problem ?

@rishijavia
Copy link
Contributor

This test will fail with how we map port currently.

@hedinasr
Copy link
Contributor Author

hedinasr commented Aug 7, 2017

This fix the problem of rebooting ! Cool !

@rishijavia
Copy link
Contributor

@Ananasr I still don't understand the problem you were trying to solve. Does the patch i proposed fixes it? In that case I can create a ticket for my patch and submit a PR for that.

@hedinasr
Copy link
Contributor Author

hedinasr commented Aug 7, 2017

If I try to use beaker-docker inside a container for testing using docker run -it -v /var/run/docker.sock:/var/run/docker.sock ruby bash (for example). Here, the container is able to connect to the Docker daemon, but beaker is trying to connect at 0.0.0.0 which is the container, not the host ... Am I clear ?

@rishijavia
Copy link
Contributor

Ahh so there are two problems here:

  1. The change of port mapping on reboot i mentioned
  2. beaker-docker not being able to run from within a container

Right? If so, we should file a ticket for both and submit PRs following the contributing guidelines for them to be verified.

@hedinasr
Copy link
Contributor Author

hedinasr commented Aug 7, 2017

Exactly ! I'll file a ticket now !

@hedinasr
Copy link
Contributor Author

hedinasr commented Aug 7, 2017

Here is my ticket https://tickets.puppetlabs.com/browse/BKR-1186

@rishijavia
Copy link
Contributor

@Ananasr couple more steps to go:

  1. Update your PR to match the contributing guideline. Update PR and commit with ticket number.
  2. Add spec tests to it

@hedinasr
Copy link
Contributor Author

hedinasr commented Aug 8, 2017

I'v some questions about writing spec tests, where can I find help ?

It was impossible to run beaker-docker inside a container
because it try to connect with SSH to 0.0.0.0 which is the
container @ip, not the host.

To fix this, we can tell to beaker-docker to connect at the
gateway IP which is the host.

- this commit add spec test and fix the commit message ;)
@rishijavia
Copy link
Contributor

@Ananasr this looks good! Kicking off CI

@rishijavia
Copy link
Contributor

@puppetlabs-jenkins test this please

Copy link
Contributor

@rishijavia rishijavia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than this change, everything else is 👍 from my side. Will merge the PR and release a new gem after the changes are made.

@@ -332,5 +336,10 @@ def find_container(host)
end.first
end

# return true if we are inside a docker container
def am_i_in_container
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in ruby we use the convention of using ? if it's a method that returns a boolean. So if you were to change this method name to in_container?, I think everything else looks great!

sorry for being a but nit picky about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem @rishijavia ! I'll fix this right now !

change "am_i_in_container" to "in_container?" to respect
ruby convention.
fix wrong call to in_container? method
@rishijavia
Copy link
Contributor

@puppetlabs-jenkins retest this please

@rishijavia rishijavia merged commit ab2987d into voxpupuli:master Aug 9, 2017
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.

3 participants