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
Access log & default-web module support in undertow #4943
Conversation
Build 237 is now running using a merge of 5ae0f4026b3bedaf1c4459cedc5da15ffaa58463 |
.addChild( | ||
builder(JspDefinition.INSTANCE) | ||
.setXmlElementName(Constants.JSP_CONFIG) | ||
.addAttributes( |
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.
Don't do this, the attributes are listed explicitly to make forward compatibility easier. When we want to create a new schema version we can just make a copy of this file, however if just reference the attributes list directly this won't work, as items may be added or removed from the list.
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.
Makes sense, I will revert this
Build 237 outcome was FAILURE using a merge of e396ffef248f22a9b927b62017bac704f9a48474 Failed tests
|
Those stacktraces look related to this PR change |
Build 246 is now running using a merge of 3fe2a5888ac23dfdd367b9885d16d3a56016af9a |
Build 247 is now running using a merge of cf30ad92995090233b7699f7a2cad69e82de8ef6 |
Build 247 outcome was SUCCESS using a merge of cf30ad92995090233b7699f7a2cad69e82de8ef6 |
@@ -169,6 +171,13 @@ | |||
<xs:attribute name="name" use="required" type="xs:string"/> | |||
<xs:attribute name="handler" use="required" type="xs:string"/> | |||
</xs:complexType> | |||
<xs:complexType name="accessLogType"> | |||
<xs:attribute name="pattern" use="required" type="xs:string"/> |
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.
This is nillable in the model, with a default of 'common'
retest this please |
Build 265 is now running using a merge of 1349f0ba47d02b718edf5ff7f29e71f697c6a022 |
Build 265 outcome was FAILURE using a merge of 1349f0ba47d02b718edf5ff7f29e71f697c6a022 Failed tests
|
Build 269 is now running using a merge of 5600ecac85642a40d06443fb8e1f0bb527a8fc74 |
@Override | ||
public void start(StartContext context) throws StartException { | ||
AccessLogService logService = accessLogService.getValue(); |
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.
This will throw an ISE if there's no injection, and there won't be if there's no access log configured. InjectedValue.getOptionalValue() won't throw the ISE.
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.
good catch, but to be fair ISE does not happen :) it just returns null.
probably bug in msc... i will fix it
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.
Interesting. I thought it odd this wouldn't have failed a test somewhere, since I figured an access log probably wasn't configured everywhere.
Build 269 outcome was SUCCESS using a merge of 5600ecac85642a40d06443fb8e1f0bb527a8fc74 |
Looks good. |
merged |
No description provided.