Skip to content

Conversation

diemol
Copy link
Member

@diemol diemol commented Dec 17, 2017

This PR has the following objectives:

  • Introduce HUB_HOST and HUB_PORT as a replacement to deprecate HUB_PORT_4444_TCP_ADDR and HUB_PORT_4444_TCP_ADDR.

    • In the entry scripts HUB_HOST and HUB_PORT will map to HUB_PORT_4444_TCP_ADDR and HUB_PORT_4444_TCP_ADDR, and when people use --link with the proposed container name in the examples selenium-hub:hub, it will continue to work.
    • HUB_PORT_4444_TCP_ADDR and HUB_PORT_4444_TCP_ADDR won't be mentioned anymore in the docs, so ideally people will migrate to this new env vars gradually.
    • If a user has the hub with the default 4444 port, then only HUB_HOST would be needed, since HUB_PORT is defaulting to 4444. The user is also able to overwrite HUB_PORT in case the hub is listening to a different port.
  • Improving the README showing a "docker network" way to start the grid and a simple docker-compose example, with the intention that users tend to use these ones more than the --link option.

Credits to @SpencerMalone and @RubyTester since most of the content comes from their work and ideas in #136 and #133.

@ddavison, could you please have a look? Let me know if something doesn't look straightforward to you.

@diemol
Copy link
Member Author

diemol commented Dec 17, 2017

We should squash and merge the PR when the review is complete, the commit history is a bit messy.

@ddavison ddavison self-assigned this Dec 20, 2017
Copy link
Member

@ddavison ddavison left a comment

Choose a reason for hiding this comment

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

all in all, looks great. just those questions there

# In the long term the idea is to remove $HUB_PORT_4444_TCP_ADDR and $HUB_PORT_4444_TCP_PORT and only work with
# $HUB_HOST and $HUB_PORT
if [ ! -z "$HUB_HOST" ]; then
HUB_PORT_PARAM=4444
Copy link
Member

Choose a reason for hiding this comment

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

confused about a couple of things, why do you check

"if $HUB_HOST isn't empty, set port to 4444"

also, i see 4 variables...

HUB_HOST, HUB_PORT, HUB_HOST_PARAM, HUB_PORT_PARAM.

what's the differences between these? can';t we consolidate? make things a little simpler?

Copy link
Member Author

@diemol diemol Dec 20, 2017

Choose a reason for hiding this comment

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

No, no... There are only 3 variables: HUB_HOST, HUB_PORT and HUB_PORT_PARAM.

What I tried to do is to check first if HUB_HOST was set, if so then use HUB_PORT_PARAM as a placeholder for port 4444 (assuming that the user started the hub with port 4444).

Therefore they only will need to set HUB_HOST and the default port will be 4444. And in case they started the hub with a different port, then HUB_PORT would be set and HUB_PORT_PARAM takes its value.

It could be simplified by doing something like:

if [ ! -z "$HUB_HOST" ] && [ ! -z "$HUB_PORT" ]; then
  echo "Connecting to the Hub using the host ${HUB_HOST} and port ${HUB_PORT}"
  HUB_PORT_4444_TCP_ADDR=${HUB_HOST}
  HUB_PORT_4444_TCP_PORT=${HUB_PORT}
fi

So the user will need to set both variables always. I just thought it could be nicer when the user only needs to set HUB_HOST since in most of the cases people will use 4444 for hub.

What do you think?

@ddavison
Copy link
Member

ddavison commented Jan 2, 2018

if this is ready to merge @diemol and you're comfortable with it, i'm comfortable with it. you can pull the trigger

@diemol
Copy link
Member Author

diemol commented Jan 2, 2018

Cool, thanks for reviewing @ddavison!
I'll merge tomorrow and prepare a release.

@diemol diemol merged commit d6dc849 into master Jan 3, 2018
@diemol diemol deleted the fixing-linking-for-newer-versions branch January 3, 2018 10:43
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.

2 participants