Fixes for BuildTrigger,SCMTrigger and jobConfigHistory incompatibility, and actions on all job pages #12
Conversation
|
I can confirm that 1 and 4 are fixed by this pull request. I've been used the proposed patch for a week now and everything seems stable. |
|
+1, @i-m-c please merge this. |
|
Hi everyone. The last three months have been quite busy here at Intel with the roll-out of an internal project that consumed the attention of our entire team. This sadly meant that we did not have the capacity to evaluate this patch or respond to any queries. Thankfully, the roll-out is over and we have been granted the time again to work on this plugin in a broader scope than just internal bugfixing. As such, you'll see many patchsets hitting the repo in the next few days. :) Additionally, we will take a look at this patch and integrate it as soon as it has been code-reviewed and passed our test-suite. Thanks for your contribution and sorry again for the long silence imposed by causes beyond our control! |
| @@ -64,7 +67,7 @@ | |||
| * @param <T> the target type of the field this helper is written for. | |||
| */ | |||
| public abstract class InheritanceGovernor<T> { | |||
| public static final Pattern runUriRegExp = Pattern.compile(".*/job/[^/]+/[0-9]+/.*"); | |||
| public static final Pattern runUriRegExp = Pattern.compile(".*/job/[^/]+/(?!configure)(?!createItem)(?!configSubmit).*"); | |||
There was a problem hiding this comment.
The blank matching rule (.*) at the end is problematic, as there might be additional sub-pages that do not need or desire inheritance to be enabled. For example, there might be "configureXYZ" subpages that only expose a subset of configuration entries. With this URL, these pages would also display fully inherited properties. This change might thus fix one bug just to create another.
It is therefore better to use this RegExp to match URLs for which it is explicitly known, that they NEED inheritance enabled, instead of the other way around. A whitelist, instead of a blacklist.
There was a problem hiding this comment.
I agree. Would it be better to add an extension point for inheritance governor 'rules' given that some plugins may add pages for which we want inheritance?
There was a problem hiding this comment.
Yes, that would be a good idea, since the plugin itself is already exposing something similar with the "InheritanceSelector" class. That one allows you to customize how externally contributed properties are handled -- whether they are duplicated, merged or only one is chosen when multiple values are encountered during inheritance.
As such, adding another extension point that contributes regular expressions for Stapler Request URLs to decide if inheritance should be enabled, disabled or kept at automatic detection, is a good idea.
|
Inheriting polling configs would be very helpful. |
|
status ? looks like development has ceased. |
|
updated to use inheritance checker extension point from #18 |
Added extension point RequestInheritanceChecker. Extensions can check if given URL points to page that needs inheritance. Added extensions, that allow inheritance in: - job main page - some trend graphs on job page (test trend, jacoco coverage trend, findbugs trend) - dashboard view and dashboard portlets
…, promoted build inheritance
…n link to work (scmPollLog)
…b-wide promotion page added by the promoted build plugin
|
rebased on current master @i-m-c, @HedAurabesh could you take a look? |
|
Any news on this? It seems like it will fix my issue: #34. |
This pull request has fixes for 4 issues:
Changed the URI regex to enable inheritance for all requested pages except config-modifying actions (see line 70). This is useful for plugins that add actions. There may be a better approach here, but this has been working for us. This will probably fix Warnings plugin doesn't show graph in Inheritance Jobs #11, Test trend result chart not shown on inherited project #13, No metrics are reported #15, but I have not testedrebased onto FIXED #13: Test trend result chart not shown on inherited project #18, and added URI inheritance checkers for promoted build and for scmPollLogHappy to discuss breaking this up if one of the above fixes doesn't cut the mustard.
Lastly, the below disclaimer is required by the lawyers:
THE FOLLOWING DISCLAIMER APPLIES TO ALL SOFTWARE CODE AND OTHER MATERIALS CONTRIBUTED IN CONNECTION WITH THIS SOFTWARE:
THIS SOFTWARE IS LICENSED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS “AS IS” AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE AND ANY WARRANTY OF NON-INFRINGEMENT, ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. THIS SOFTWARE MAY BE REDISTRIBUTED TO OTHERS ONLY BY EFFECTIVELY USING THIS OR ANOTHER EQUIVALENT DISCLAIMER IN ADDITION TO ANY OTHER REQUIRED LICENSE TERMS.
ONLY THE SOFTWARE CODE AND OTHER MATERIALS CONTRIBUTED IN CONNECTION WITH THIS SOFTWARE, IF ANY, THAT ARE ATTACHED TO (OR OTHERWISE ACCOMPANY) THIS SUBMISSION (AND ORDINARY COURSE CONTRIBUTIONS OF FUTURES PATCHES THERETO) ARE TO BE CONSIDERED A CONTRIBUTION. NO OTHER SOFTWARE CODE OR MATERIALS ARE A CONTRIBUTION.