-
Notifications
You must be signed in to change notification settings - Fork 17
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
Document restart not required #6
Document restart not required #6
Conversation
*/ | ||
private void registerFilter() throws ServletException { | ||
this.filter = new KerberosSSOFilter(createConfigMap(), new SpnegoKerberosAuthenticationFactory()); | ||
PluginServletFilter.addFilter(filter); |
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.
But here you are effectively just adding a new instance to a list of filters already containing older instances with the old config no?
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.
In this method, yes. The calling code is supposed to deal with that by removing the filter first in case it is re-adding.
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 particular refactorization just replaced a verbatim copy of the same code present on both places already. I am quite confident about this part of the patch.
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.
Yes, the refactoring is fine, I just wanted to point out where my concern was about that a restart might still be required. But I trust your judgement :)
I have retested with end-to-end tests locally: jenkinsci/acceptance-test-harness#192 |
I do not see a reason why restart would be required after dd64a55. My tests does neither.