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.

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

add add-hosts #927

Merged
merged 1 commit into from Sep 21, 2018
Merged

add add-hosts #927

merged 1 commit into from Sep 21, 2018

Conversation

magicsong
Copy link
Contributor

@magicsong magicsong commented Sep 19, 2018 •
edited

Add a --add-host flag，same as --add-host in docker run . It is very useful when user in LAN or in dev environment.

@openshift-ci-robot openshift-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 19, 2018
@magicsong
Copy link
Contributor Author

magicsong commented Sep 19, 2018 •
edited

fixes #921

@bparees
Copy link
Contributor

bparees commented Sep 19, 2018

please add a test that confirms the hosts are added to the container at assemble time (which i assume is your goal).

@bparees bparees self-assigned this Sep 19, 2018
@bparees
Copy link
Contributor

bparees commented Sep 19, 2018

/ok-to-test

@openshift-ci-robot openshift-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 19, 2018
@bparees
Copy link
Contributor

bparees commented Sep 19, 2018

[test]

@openshift-bot
Copy link
Contributor

Evaluated for source to image test up to 1ca77a1

@openshift-bot
Copy link
Contributor

Source To Image Test Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_s2i/777/) (Base Commit: 8f7dcac) (PR Branch Commit: 1ca77a1)

@magicsong
Copy link
Contributor Author

example: s2i build https://github.com/openshift/ruby-hello-world centos/ruby-23-centos7 --add-host rubygems.org:172.0.0.1 test-ruby-app. With --add-host, this sample will fail because of "rubygems.org" has been led to local. Without --add-host, it will work as expected.

@bparees
Copy link
Contributor

bparees commented Sep 20, 2018

example: s2i build https://github.com/openshift/ruby-hello-world centos/ruby-23-centos7 --add-host rubygems.org:172.0.0.1 test-ruby-app. With --add-host, this sample will fail because of "rubygems.org" has been led to local. Without --add-host, it will work as expected.

great, please add that scenario as a test to https://github.com/openshift/source-to-image/blob/master/hack/test-stirunimage.sh

so we can make sure it continues to work.

@magicsong
Copy link
Contributor Author

@bparees The scenario has been added to test-stirunimage.sh. I also change 172.0.0.1 to 1.1.1.1 because it takes too long to wait the job to be failed. The job stops immediately when using public ip.

@bparees
Copy link
Contributor

bparees commented Sep 20, 2018

thanks @magicsong, can you squash your commits and i'll merge?

coreydaley
coreydaley reviewed Sep 20, 2018
View reviewed changes
hack/test-stirunimage.sh Outdated
@@ -136,6 +136,11 @@ docker run test >& "${WORK_DIR}/s2i-override-run.log"
grep "Running custom run" "${WORK_DIR}/s2i-override-run.log"
check_result $? "${WORK_DIR}/s2i-override-run.log"

test_debug "s2i build with add-host option"
s2i build https://github.com/openshift/ruby-hello-world centos/ruby-23-centos7 --add-host rubygems.org:1.1.1.1 test-ruby-app &> "${WORK_DIR}/s2i-add-host.log"
grep "Build failed" "${WORK_DIR}/s2i-add-host.log"
Copy link
Member

Choose a reason for hiding this comment

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

Can this check for a more specific failure reason related to the rubygems.org host not being found?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, but the error will change if 1.1.1.1 is down or close its http/https ports. I change the test scenario testing both cases----with and without --add-host flag---to make sure the failure is caused by --add-host flag

coreydaley
coreydaley reviewed Sep 20, 2018
View reviewed changes
hack/test-stirunimage.sh
@@ -136,6 +136,12 @@ docker run test >& "${WORK_DIR}/s2i-override-run.log"
grep "Running custom run" "${WORK_DIR}/s2i-override-run.log"
check_result $? "${WORK_DIR}/s2i-override-run.log"

test_debug "s2i build with add-host option"
Copy link
Member

Choose a reason for hiding this comment

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

I think that this should be two separate tests, less one give the wrong result and they both pass.
Also, it should give some kind of specific error if rubygems.org can not be found.

Copy link
Member

Choose a reason for hiding this comment

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

You could set rubygems.org:127.0.0.1 and it should fail (since localhost is not a rubygems server) and you should get an error message about rubygems.org not being "accessible"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i have change to 0.0.0.0 because error of 172.0.0.1 is much more unclear and it takes more time because it is the LOOPBACK internet interface. I also squash all my commits. @bparees

@bparees
Copy link
Contributor

bparees commented Sep 21, 2018

[merge]

bparees
bparees reviewed Sep 21, 2018
View reviewed changes
hack/test-stirunimage.sh Outdated
@@ -136,6 +136,11 @@ docker run test >& "${WORK_DIR}/s2i-override-run.log"
grep "Running custom run" "${WORK_DIR}/s2i-override-run.log"
check_result $? "${WORK_DIR}/s2i-override-run.log"

test_debug "s2i build with add-host option"
s2i build https://github.com/openshift/ruby-hello-world centos/ruby-23-centos7 --add-host rubygems.org:0.0.0.0 test-ruby-app &> "${WORK_DIR}/s2i-add-host.log"
Copy link
Contributor

Choose a reason for hiding this comment

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

since you expect this to fail you need an || true at the end, because we run our script with exit on error.

Copy link
Contributor Author

@magicsong magicsong Sep 21, 2018 •
edited

Choose a reason for hiding this comment

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

ok, I added a set +e before running my test.

bparees
bparees reviewed Sep 21, 2018
View reviewed changes
pkg/cmd/cli/cmd/build.go Outdated
@@ -215,5 +215,6 @@ $ s2i build . centos/ruby-22-centos7 hello-world-app
buildCmd.Flags().StringVar(&(networkMode), "network", "", "Specify the default Docker Network name to be used in build process")
buildCmd.Flags().StringVarP(&(cfg.AsDockerfile), "as-dockerfile", "", "", "EXPERIMENTAL: Output a Dockerfile to this path instead of building a new image")
buildCmd.Flags().BoolVarP(&(cfg.KeepSymlinks), "keep-symlinks", "", false, "When using '--copy', copy symlinks as symlinks. Default behavior is to follow symlinks and copy files by content")
buildCmd.Flags().StringArrayVar(&cfg.AddHost, "add-host", []string{}, "Specify additional entry added to /etc/hosts in container, multiple --add-host can be used to add multiple entry")
Copy link
Contributor

Choose a reason for hiding this comment

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

grammar. should be:
"Specify additional entries to add to the /etc/hosts in the assemble container, multiple --add-host arguments can be used to add multiple entries"

@bparees
Copy link
Contributor

bparees commented Sep 21, 2018

[merge]

@openshift-bot
Copy link
Contributor

Evaluated for source to image merge up to 1b45714

@openshift-bot
Copy link
Contributor

openshift-bot commented Sep 21, 2018 •
edited

Source To Image Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_s2i/779/) (Base Commit: 8f7dcac) (PR Branch Commit: 1b45714)

@openshift-bot
Copy link
Contributor

Source To Image Action Required: Please contact #openshift-dev to have this pull request manually reviewed and tested

@openshift-bot openshift-bot merged commit ec6ef8b into openshift:master Sep 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bparees bparees bparees left review comments

@coreydaley coreydaley coreydaley left review comments

Assignees

@bparees bparees

Labels
size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@magicsong @bparees @openshift-bot @coreydaley @openshift-ci-robot