Pre-init s2i extensibility #32
Conversation
|
Can one of the admins verify this patch? |
1 similar comment
|
Can one of the admins verify this patch? |
|
[test] |
|
Looks like I've made a mistake somewhere, will fix ASAP |
|
@hhorak I have added a fix, could you please trigger the tests again? |
|
[test] |
|
ping, can someone please review this PR? |
| if [ -d ./nginx-pre-init ]; then | ||
| echo "---> Copying nginx pre-init scripts..." | ||
| if [ "$(ls -A ./nginx-pre-init/*)" ]; then | ||
| cp -v ./nginx-pre-init/* "${NGINX_APP_ROOT}/etc/nginx-pre-init" |
There was a problem hiding this comment.
Any reason why to move pre-init scripts there?
I'm not against it, maybe I'm missing something.
Otherwise /opt/app-root/src seems to me as better location that /opt/app-root/etc - the etc is misleading...
But generate_container_user and scl_enable are already in /opt/app-root/etc, so no big harm. But I vote for moving these scripts into NGINX_CONTAINER_SCRIPTS_PATH.
There was a problem hiding this comment.
Yeah, that makes more sense. I've used this location before, don't know why I chose something else here than NGINX_CONTAINER_SCRIPTS_PATH. Will change that
There was a problem hiding this comment.
I've changed this in #41, since I'm not allowed to push to this branch.
| cleanup_test_app | ||
| } | ||
|
|
||
| test_pre_init_script() { |
There was a problem hiding this comment.
Definitely +1 for passing app name as argument to functions. So be able to use these functions in more cases.
test_pre_init_script and test_application
test_connection_s2i and test_connection
are very similar. Only functions used for s2i app testing adds 1-2 lines of code.
I suggest:
1. remove cleanup_test_app from test_application and have to call it explicitly after testing of each app finished.
(2. rename test_application to test_application_basics)
3. when testing s2i app use same test_application and before calling cleanup_test_app do
test_command "cat /opt/app-root/etc/nginx.d/default.conf | grep 'resolver' | sed -e 's/resolver\(.*\);/\1/' | grep 'DNS_SERVER'" ""
check_result $? "pre-init-test-app"
test_for_output "http://$(container_ip):${test_port}/aliased/index2.html" "NGINX2 is working"
There was a problem hiding this comment.
+1, will implement this. Just a note, it's the other way around, test_connection has one more line to test. But i got your point, and I'll fix my code.
There was a problem hiding this comment.
I haven't changed this in #41, because I'd rather see using the test.lib.sh from common repo.
|
Can one of the admins verify this patch? |
1 similar comment
|
Can one of the admins verify this patch? |
|
I wanted to push changes into this PR directly, but it didn't work, so I opened a new PR #41, using most of the changes here, but also doing slightly more. |
Hi,
This PR would add ability to execute user supplied scripts just before nginx start. This would fix #18, because it implements #19, and it would also fix #20.
All user has to do is place
.shscripts in./nginx-pre-init/folder, and these scripts will then be sourced right before nginx starts.I have also modified permissions on config files, for when container is not running on user 1001 (as seen in tests)
This PR also includes test case for each version and modified test running script to be able to run more than one test.
I've separated changes into three commits for each version to track changes more transparently. Added functionality is the same in all of them, except for 1.8, where I decided to add option to extend the default server block, which I took from 1.10 & 1.12.