-
Notifications
You must be signed in to change notification settings - Fork 170
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
THREESCALE-7906 #1319
THREESCALE-7906 #1319
Conversation
16455e3
to
d5844f9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nginx part LGTM, I cannot understand the circleCI changes.
@@ -146,7 +146,8 @@ jobs: | |||
- run: make test-runtime-image gateway-logs --keep-going | |||
- login-docker: | |||
command: | | |||
IMAGE_TAG="${CIRCLE_TAG:-${CIRCLE_BRANCH}}" | |||
CIRCLE_BRANCH_LOWER=`echo $CIRCLE_BRANCH | tr '[:upper:]' '[:lower:]'` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think that these changes should be part of this PR, maybe a separate commit or separate PR. Why is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The s2i-runtime failed because it used the branch name as a tag for the docker image, but it fails with uppercase characters. This change is to transform them to lowercase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I separated the circleci changes on a different commit
d5844f9
to
b663554
Compare
gateway/http.d/apicast.conf.liquid
Outdated
@@ -4,6 +4,15 @@ map $status $extended_access_log { | |||
default ''; | |||
} | |||
|
|||
map $status $access_logs_enabled { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maps works like switch..case selectors to set values to conditional variables in nginx, if I am not mistaken. If I understand correctly then, when default
is provided alone, the value of the variable will be the default value (when even default
is omitted, it will be the empty string, by the way).
So why using $status
? Out of curiosity, would something like the following work instead?
map $status $access_logs_enabled { | |
map "" $access_logs_enabled { |
Does this PR need any further work or review? |
Under some error circumstances (like 414 Request-URI Too Large) the access log were generated in a phase previous to having defined the access_log_enabled and the extended_access_logs_enabled. This causes a warning to arise, but most importanty, prevented that acces to be logged. We use the workarround of declaring the variables as a map with a default value because maps can be used in the http context and therefore will be available and initialized even on the given error case.
b663554
to
22e39cb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
THREESCALE-7906
Under some error circumstances (like 414 Request-URI Too Large) the access log were generated in a phase previous to having defined the access_log_enabled and the extended_access_logs_enabled.
This causes a warning to arise, but most importanty, prevented that acces to be logged.
We use the workarround of declaring the variables as a map with a default value because maps can be used in the http context and therefore will be available and initialized even on the given error case.