Skip to content

Moving config generation in docker files #502

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 4, 2017
Merged

Moving config generation in docker files #502

merged 2 commits into from
Jul 4, 2017

Conversation

Remi-p
Copy link
Contributor

@Remi-p Remi-p commented Jun 19, 2017 •
edited
Loading

Hi,
Following the issue Permission denied issue when starting node-chrome pod #3 from ddavison/selenium-openshift-templates, I propose to make the same modification type as in move the creation of the config.json file into a RUN directive #185.

Typically, moving this line from NodeBase/entry_point.sh :

/opt/selenium/generate_config > /opt/selenium/config.json

To the end of the dockerfiles (with RUN directive).

make generate_all, make all, & make test have been run. Also I tried the built node-chrome image with OpenShift, it works (see this temporary Docker Hub image).

@Remi-p
Moving config generation in docker files
0dd193a 
This allow random users to launch the images.
The behavior is needed for OpenShift.
Cf. issue 3 from ddavison/selenium-openshift-templates
@ddavison
Copy link
Member

yep! i'm good with this. please do me favor though, and take out your Dockerfile changes? please only change the Dockerfile.txt. In the header of Dockerfile, it kindly specifies not to edit it. This is because this file is generated.

I'll merge this in as soon as that is fixed! thanks @Remi-p ! nice work.

@ddavison ddavison self-assigned this Jun 19, 2017
@Remi-p
Copy link
Contributor Author

Remi-p commented Jun 19, 2017

My bad I thought the warning was for local modifications...
Should I rebase the two commits together or is it OK ?

@ddavison
Copy link
Member

don't worry about squashing. thanks @Remi-p !

@Remi-p Remi-p mentioned this pull request Jun 26, 2017
Merged
@Remi-p
Copy link
Contributor Author

Remi-p commented Jul 3, 2017

Hey ! Sorry to bother you, but should I do another modification ? I say that because of the awaiting-external-changes label.
Thanks for your reply !

@ddavison ddavison merged commit 45304b7 into SeleniumHQ:master Jul 4, 2017
@ddavison
Copy link
Member

ddavison commented Jul 4, 2017

oops, thanks for reminding me!

@Remi-p
Copy link
Contributor Author

Remi-p commented Jul 4, 2017

No problem!

@Remi-p
Copy link
Contributor Author

Remi-p commented Jul 29, 2017

It was for correcting a permission problem (see Permission denied issue when starting node-chrome pod). But you're right this way it breaks the use of environment variables.
A solution might be to generate the config file at build time, like it is done here, and regenerate it at run time ? As it is done with the Hub. This way, people having permissions problems will have a default configuration, and the ones wanting to customize the variables will have the opportunity.

Remi-p added a commit to Remi-p/docker-selenium that referenced this pull request Aug 3, 2017
@Remi-p
Config generation at runtime + buildtime
8dfb2ae 
See SeleniumHQ#502
Generation at build time = default configuration file
Generation at run time = allow to pass environment variables
Remi-p added a commit to Remi-p/docker-selenium that referenced this pull request Aug 3, 2017
@Remi-p
Config generation at runtime + buildtime
7c31de1 
See SeleniumHQ#502
Generation at build time = default configuration file
Generation at run time = allow to pass environment variables
@Remi-p Remi-p mentioned this pull request Aug 3, 2017
Merged
1 task
testphreak added a commit to testphreak/docker-selenium that referenced this pull request Sep 11, 2017
@testphreak
Fix for issue with not being able to pass environment variables into …
033214c 
…node containers at runtime

When changes were introduced to generate default config during build
time with SeleniumHQ#502 and then with SeleniumHQ#529, it broke the ability to pass in
environment variables to the node containers at runtime. This fix aims
to provide the ability to do both i.e. continue to pass environment
variables at runtime and generate a default config at build time for
the node.
@testphreak testphreak mentioned this pull request Sep 11, 2017
Closed
1 task
@ddavison ddavison mentioned this pull request Sep 13, 2018
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@ddavison ddavison

Labels
R-awaiting-external-changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Remi-p @ddavison @junhui