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

Log httpd log_error to stderr #73

Merged
merged 2 commits into from
Mar 2, 2016
Merged

Log httpd log_error to stderr #73

merged 2 commits into from
Mar 2, 2016

Conversation

ppisar
Copy link
Contributor

@ppisar ppisar commented Mar 2, 2016

This patch set fixes issue #72.

Previously, httpd logged access log to "cat" command and error log to
/tmp/error_log. To have the both logs available from TTY, the error
log is file description sclorg#1. The access log was simplified to file
description #0.

<sclorg#72>
@mfojtik
Copy link
Contributor

mfojtik commented Mar 2, 2016

LGTM

mfojtik pushed a commit that referenced this pull request Mar 2, 2016
Log httpd log_error to stderr
@mfojtik mfojtik merged commit 2ab4421 into sclorg:master Mar 2, 2016
@rhcarvalho
Copy link
Contributor

@ppisar nice patch 👏 ! Thanks! :)

@mfojtik the original issue was left open. BTW we need this for PHP and perhaps other httpd-based images as well...

@@ -185,6 +205,8 @@ test_application() {
# must return (or terminate with) non-zero value to report an failure,
# 0 otherwise.
# Third argument is s2i --env argument value.
# The test function have available container ID in $cid_file, container output
# in ${tmp_dir}/out, countainer stderr in ${tmp_dir}/err.
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit: s/countainer/container

@rhcarvalho
Copy link
Contributor

Well, too late, but worth asking: could we get the tests for both image versions? (5.16 is missing)

@ppisar
Copy link
Contributor Author

ppisar commented Mar 3, 2016

On Wed, Mar 02, 2016 at 02:51:41PM -0800, Rodolfo Carvalho wrote:

Well, too late, but worth asking: could we get the tests for both image
versions? (5.16 is missing)

The 5.20 has different test "frame work". 5.16 tests are not extendable.

Actually the tests should be rewritten to something more suitable for testing
that supports nesting subtests, reporting failure reason, parallel testing
etc. E.g. Perl's Test::More. But I don't know if that were acceptable for
OpenShift. I've never seen what OpenShift test environment provides,
guarantees or expects.

-- Petr

@ppisar ppisar deleted the stderr branch March 3, 2016 07:00
@rhcarvalho
Copy link
Contributor

Actually the tests should be rewritten to something more suitable for testing
that supports nesting subtests, reporting failure reason, parallel testing
etc.

I totally agree 👍

The tests in this repository are OpenShift-agnostic, they require just docker.

However, we need to take a more holistic view when talking about a change like rewriting.
Historically, they started as a "common" "framework" that is generated by s2i create. That command creates a scaffolding for creating new S2I builder images.

Through the past year, when working on the builder images and trying to improve the tests, I was reminded to keep the consistency among the images we provide, which is good in one hand, but hard to maintain over time. And as you just pointed out, Perl tests for 5.16 and 5.20 went out of sync...

(and there is really another problem in there, copy-paste...)

IMHO Bash is great for short scripts, but is a very poor tool for writing the day-after-day more complex scripts that empower the images, and very poor for writing tests.

Even though Perl's Test::More is very cool, I doubt we have enough Perl know-how across the team to make it the "new language" for the scripts in all images.

Given what's in the base OS image, I'd say our choices are Bash, Perl, Python.

@mfojtik
Copy link
Contributor

mfojtik commented Mar 3, 2016

@rhcarvalho the tests also depend on s2i binary.

@mfojtik
Copy link
Contributor

mfojtik commented Mar 3, 2016

@rhcarvalho also as you said, few of us have confidence in perl or python or ruby to start writing tests for images. We do all do Go, but that leaves SCL folks out of loop because that is something they don't do. So even it sound silly, the "bash" is the only common framework we share currently :-) And I'm OK improving it or making the testing framework better.

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.

None yet

3 participants