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

Do not register XMLEntityResolver as a global extension #65

Conversation

@jerrinot
Copy link
Contributor

jerrinot commented Mar 16, 2017

The resolver is useful only for parsing TestNG results. There
is no need to register the resolver as a global extension.

Please see a related discussion and reasoning at jenkinsci/jenkins#2806

The resolver is useful only for parsing TestNG results. There
is no need to register the resolver as a global extension.
@jerrinot jerrinot changed the title Do not register XMLEntityResolver as global extension Do not register XMLEntityResolver as a global extension Mar 16, 2017
@jerrinot

This comment has been minimized.

Copy link
Contributor Author

jerrinot commented Mar 27, 2017

hi,

any chance to have this PR reviewed?
Thanks!

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Mar 27, 2017

It is hard to evaluate the impact of this change. JUnit plugin used to be a part of the Jenkins core, hence there may be various barely traceable dependencies on the original functionality. I would recommend running Jenkins ATH and PCT test suites before merging this change

@jerrinot

This comment has been minimized.

Copy link
Contributor Author

jerrinot commented Mar 27, 2017

@oleg-nenashev: thanks for your feedback, very much appreciated. who can run these test suites?

@olivergondza

This comment has been minimized.

Copy link
Member

olivergondza commented Mar 27, 2017

I would say that if some plugin depends on that to be set globally, it would start failing as soon as junit is no longer always installed (IOW, Jenkins 2.0). What can be tricky to detect is such expectation by a plugin that depends on junit plugin.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Mar 27, 2017

I would say that if some plugin depends on that to be set globally, it would start failing as soon as junit is no longer always installed (IOW, Jenkins 2.0)

It was possible to disable it even before 2.0. The plugin has been decoupled around .509.x IIRC. But Jenkins retains all the implicit dependencies via the detachedPlugins engine, hence Jenkins won't startup if somebody deletes the JUnit plugin && there is a transient dependency via an old core dependency in a plugin. But I agree it is hard to trace the usages.

thanks for your feedback, very much appreciated. who can run these test suites?

Good question. @olivergondza and @andresrc could probably suggest the best ways to do so without rebuilding the world

@jerrinot

This comment has been minimized.

Copy link
Contributor Author

jerrinot commented Mar 27, 2017

If this change it too risky then I can resurrect the original PR
perhaps with some maximum time-to-live.

The current situation is really suboptimal to put it nicely - there is a RPC for every single XML being processed.

@olivergondza

This comment has been minimized.

Copy link
Member

olivergondza commented Mar 29, 2017

To my understanding this is triggered iff it is a test case referring to TestNG DTD, which I presume does not happen often. To verify this is safe we need 2 test cases to get such testcase.xml parsed by both the plugins successfully, verify it would fail without the global resolver (the feature is completely untested) and verify it gets back fixed once the "local" resolver is hooked back as this PR suggests.

The only plugin specifically interested in TestNG results seems to be testng-plugin so the impact should not be that hard to verify.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Apr 13, 2017

Since there is not that much prgress with testing, do we want to go forward and release this change as release-candidate to make it testable in the wild?

Copy link
Member

oleg-nenashev left a comment

Nobody responds, so 👍

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Apr 21, 2017

But I agree the autotests requested by @olivergondza is a right precondition for the merge

@jerrinot

This comment has been minimized.

Copy link
Contributor Author

jerrinot commented Apr 21, 2017

From what I can see the extension is meant to prevent HTTP calls to fetch TestNG DTD when parsing TestNG output files.

So as long as the machine running the parser has an access to http://testng.org/testng-1.0.dtd it will work even when the extension is not registered at all -> that makes it hard to write an automated test.

One would need to intercept remote calls I guess?

@jerrinot

This comment has been minimized.

Copy link
Contributor Author

jerrinot commented Apr 21, 2017

This PR is effectively reverting the code to the same state as it was when the resolver was originally introduced back in 2007. It was only in 2011 when the concept was generalized and the resolver was turn into an Extension.

Unfortunately having it as an Extension goes directly against the original goal - decrease number of remote calls - when running in the master/slave mode. Here is the original discussion: http://jenkins-ci.361315.n4.nabble.com/Re-Online-XML-validation-td367533.html

@olivergondza

This comment has been minimized.

Copy link
Member

olivergondza commented Apr 25, 2017

I see the testing is a pain here and this clearly is an improvement. To my understanding it can negatively impact only instances that are resolving that entity in different context and can not reach the testng website.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Apr 25, 2017

@rsandell was going to take a look, so I have conditionally assigned him to the ticket

@jerrinot

This comment has been minimized.

Copy link
Contributor Author

jerrinot commented May 2, 2017

hello @rsandell, did you have a chance to have a look / test it?

@varju

This comment has been minimized.

Copy link

varju commented May 25, 2017

Hi there. Sorry to drop in uninvited, but I landed here based on the discussion in jenkinsci/jenkins#2806.

From the comments above, it sounds like this change is approved by all. Is there anything blocking the merge?

@jerrinot

This comment has been minimized.

Copy link
Contributor Author

jerrinot commented May 30, 2017

many thanks to all reviewers!
is there anything preventing to merge this changeset?

@abayer

This comment has been minimized.

Copy link
Member

abayer commented Jun 7, 2017

Nope, there isn't. =) @oleg-nenashev Want me to merge and do a beta release?

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Jun 8, 2017

@jerrinot

This comment has been minimized.

Copy link
Contributor Author

jerrinot commented Jun 23, 2017

@abayer, @oleg-nenashev: guys, who can merge it? :)

@olivergondza olivergondza merged commit 03da1e4 into jenkinsci:master Jun 23, 2017
1 check passed
1 check passed
Jenkins This pull request looks good
Details
@jerrinot jerrinot deleted the jerrinot:improvements/entity-resolver-local-only branch Jun 23, 2017
@jerrinot

This comment has been minimized.

Copy link
Contributor Author

jerrinot commented Jun 23, 2017

🎆 thanks!

olivergondza added a commit that referenced this pull request Aug 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.