[FIXED JENKINS-39553] Make GitHub plugin BuildableItem aware #153

stephenc · 2016-11-07T12:36:35Z

@reviewbybees @jenkinsci/code-reviewers

This change is

ghost · 2016-11-07T13:35:01Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

oleg-nenashev

🐛 for the undocumented AbstractMethodError in the code, which seems to be a breaking change. Other stuff LGTM excepting NITs

oleg-nenashev · 2016-11-08T10:54:34Z

src/main/java/com/cloudbees/jenkins/Cleaner.java


    /**
     * Called when a {@link GitHubPushTrigger} is about to be removed.
+     * @deprecated use {@link #onStop(Item)}


Also add the @Deprecated annotation

oleg-nenashev · 2016-11-08T10:55:39Z

src/main/java/com/cloudbees/jenkins/GitHubPushTrigger.java

        @SuppressWarnings("unused")
-        public FormValidation doCheckHookRegistered(@AncestorInPath Job<?, ?> job) {
-            Preconditions.checkNotNull(job, "Job can't be null if wants to check hook in monitor");
+        public FormValidation doCheckHookRegistered(@AncestorInPath Item job) {


Would be useful to add @Restricted when you touch such methods. NIT

oleg-nenashev · 2016-11-08T10:56:22Z

src/main/java/com/cloudbees/jenkins/GitHubRepositoryNameContributor.java

-     *
-     * @return true if overridden deprecated method
+     * Looks at the definition of {@link Item} and list up the related github repositories,
+     * then puts them into the collection.


oleg-nenashev · 2016-11-08T10:59:36Z

src/main/java/com/cloudbees/jenkins/GitHubRepositoryNameContributor.java

+        )) {
+            // if this impl is legacy, it cannot contribute to non-jobs, so not an error
+            if (item instanceof Job) {
+                parseAssociatedNames((Job<?, ?>) item, result);


Maybe put a warning on the FINE level? Just in case somebody needs to debug the compat issue

oleg-nenashev · 2016-11-08T11:09:04Z

src/main/java/com/cloudbees/jenkins/GitHubRepositoryNameContributor.java

+                parseAssociatedNames((AbstractProject<?, ?>) item, result);
+            }
+        } else {
+            throw new AbstractMethodError("you must override the new overload of parseAssociatedNames");


🐛 So it will expose the error when somebody uses Predicates like withTrigger e.g. in DefaultPushGHEventSubscriber.#isApplicable(). Instead of returning false this method will start throwing Error after the patch if there is an incompatible contributor. Just if I read it right...

IMHO there should be one of the following fixes:

Log warning and do nothing

Replace the Error by non-runtime exception and implement correct handling in the upper logic

Actually this is more a design error in the original API. These methods taking the collection should actually have been protected as they form part of the SPI not the API.

If you have an old implementation, then you will have overridden parseAssociatedNames(Job,Collection) or parseAssociatedNames(AbstractProject,Collection) and thus we delegate to those.

If you have a new implementation, then you are supposed to override parseAssociatedNames(Item,Collection) and if you don't then this exception should blow up in your face and give you the AME.

If we had these SPI methods as protected then we could make parseAssociatedNames(Item,Collection) abstract and use the API methods like parseAssociatedNames(Item) to route through to the implementation of existing extension points... but instead, as this is public API we have to do it this way and we have no way to force new implementations to override parseAssociatedNames(Item,Collection) other than this exception

Also please check the previous implementation of parseAssociatedNames(Job<?, ?> job, Collection<GitHubRepositoryName> result) which threw the exact same exception. All I am doing here is moving the exception throwing from the previously most generic method to the new most generic method.

The exception will only be thrown if you have an implementation that does not override any parseAssociatedNames methods. If this had been designed as an SPI then the implementation methods would be protected and we could have shifted the abstract and used a guard.

To clarify for @KostyaSha this is not a bug just migration of existing pattern

@stephenc only since

oleg-nenashev · 2016-11-08T11:10:41Z

src/main/java/org/jenkinsci/plugins/github/extension/GHEventsSubscriber.java

 */
 public abstract class GHEventsSubscriber implements ExtensionPoint {
    private static final Logger LOGGER = LoggerFactory.getLogger(GHEventsSubscriber.class);
+    private transient Boolean hasIsApplicableItem;


I would explicitly add CheckForNull, just for the future

oleg-nenashev · 2016-11-08T11:11:02Z

src/main/java/org/jenkinsci/plugins/github/util/FluentIterableWrapper.java

+     * Returns the elements from this fluent iterable that are instances of the supplied type. The
+     * resulting fluent iterable's iterator does not support {@code remove()}.
+     */
+    @CheckReturnValue


oleg-nenashev · 2016-11-08T11:11:48Z

src/main/java/org/jenkinsci/plugins/github/util/JobInfoHelpers.java

-        return new Predicate<Job>() {
-            public boolean apply(Job job) {
-                return triggerFrom(job, clazz) != null;
+    public static <ITEM extends Item> Predicate<ITEM> withTrigger(final Class<? extends Trigger> clazz) {


I'm not 100% sure it's a binary compatible change, same for below. Need to check it

The type gets erased. It may cause compiler errors if compiling against the newer API but the erased type is still Predicate

oleg-nenashev · 2016-11-08T11:12:32Z

src/main/java/org/jenkinsci/plugins/github/util/JobInfoHelpers.java

+     * TODO use standard method in 1.621+
+     */
+    @CheckForNull
+    public static <T extends Trigger> T triggerFrom(Item item, Class<T> tClass) {


oleg-nenashev · 2016-11-08T11:12:47Z

src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java

+     * @return runnable to create hooks on run
+     * @see #createHookSubscribedTo(List)
+     */
+    public Runnable registerFor(final Item item) {


since missing

stephenc · 2016-12-07T23:29:53Z

@lanwen can we get this merged and released?

KostyaSha · 2016-12-07T23:31:57Z

@stephenc when it will be reviewed and verified

stephenc · 2016-12-07T23:45:58Z

@KostyaSha the changes are good, oleg just misunderstood... right now this is required to get the github-branch-source 2.0 release out.

KostyaSha · 2016-12-07T23:52:15Z

@stephenc situation is that all existing github plugins was ignored, we also suggested to use shared connection and other things, but your multibranch still inventing own things. Now different stuff is pushed into plugins and we will be forced to maintain it. That's look not very good, my gh plugins fully using github-plugin so i need firstly identify that it wouldn't break anything. github-api-plugin already got portion of sh.. without answers jenkinsci/github-api-plugin@185541a and i already raised questions to jenkinsci board why non-maintainer people making changes into plugin.

@stephenc i will try review as soon as i can 3 am 💤

stephenc · 2016-12-08T01:21:31Z

@KostyaSha I am not happy with some of the mess that I have found, e.g. GitHub org folders plugin should never have been released... I'm working on getting these aligned better, but it's a marathon not a sprint.

KostyaSha

Oleg's comments are not addressed

KostyaSha · 2016-12-08T13:02:31Z

@KostyaSha I am not happy with some of the mess that I have found,

Compare what is done in CB plugins and what exists and working well for a year here https://github.com/KostyaSha/github-integration-plugin (it was under jenkinsci and was planned to go into one powerful github-plugin, but i was forced to go out of jenkins infra).

stephenc · 2016-12-08T21:40:24Z

@KostyaSha said:

Oleg's comments are not addressed

Incorrect, I fixed or added a rationale for every one of oleg's comments

stephenc · 2016-12-08T21:41:27Z

FTR this is now blocking releasing Branch API 2.0, so if we could get some progress on a decision it would be super awesome as I have a lot of plugins to juggle getting releases at or near the same time

KostyaSha · 2016-12-08T21:42:05Z

And please don't merge branches into branch this complicates search in tree.

stephenc · 2016-12-08T21:50:11Z

Sorry merge commit got pushed by accident when I was trying to diagnose the events being missing (thank you fail2ban... though not having these changes was also a culprit, fail2ban was stopping them even getting to my server)

KostyaSha · 2016-12-08T22:08:00Z

src/main/java/com/cloudbees/jenkins/GitHubPushTrigger.java

-        public FormValidation doCheckHookRegistered(@AncestorInPath Job<?, ?> job) {
-            Preconditions.checkNotNull(job, "Job can't be null if wants to check hook in monitor");
+        @Restricted(NoExternalUse.class) // invoked from Stapler
+        public FormValidation doCheckHookRegistered(@AncestorInPath Item job) {


rename job to item

KostyaSha · 2016-12-08T22:10:19Z

src/main/java/com/cloudbees/jenkins/GitHubRepositoryNameContributor.java

    public static class FromSCM extends GitHubRepositoryNameContributor {
        @Override
-        public void parseAssociatedNames(Job<?, ?> job, Collection<GitHubRepositoryName> result) {
+        public void parseAssociatedNames(Item job, Collection<GitHubRepositoryName> result) {


job -> item

oleg-nenashev

🐝 since it does not increase the technical debt. But this Abstract method handling stuff ideally needs to be solved.

stephenc · 2016-12-09T12:56:28Z

@oleg-nenashev yes, a correct solution is to use the SPI pattern like I have done in SCM API where the methods that implementers are required to implement are protected and the methods that consumers are supposed to call are public final.

Then when you need to adapt the API/SPI, what you do is have the public final check the current protected abstract and fall back through previous SPI methods. You will be guaranteed to get a match as:

A new implementation will not compile unless they implement the current protected abstract
An old implementation has to have implemented one of the previous protected abstract that you have now deprecated or they would not have compiled against the old SPI

stephenc · 2016-12-09T12:56:55Z

@reviewbybees done

KostyaSha · 2016-12-11T20:01:41Z

Have you tried open jenkins global settings?

stephenc · 2016-12-11T20:10:54Z

@KostyaSha you mean the Caused by: java.lang.NoSuchMethodError: jenkins.model.Jenkins.getActiveInstance()Ljenkins/model/Jenkins; errors because this needs the bump to 1.609 baseline to work with credentials (which is already in master but you didn't want me to include the merge commit?

KostyaSha · 2016-12-11T20:12:51Z

I placed new gh plugin version into my test instance and it failed with

target/tmp/webapp/WEB-INF/lib/jenkins-core-1.625.3.jar!/lib/form/rowSet.jelly:47:23: <d:invokeBody> No such property: configuration for class: org.jenkinsci.plugins.github.GitHubPlugin
	at

stephenc · 2016-12-11T20:15:12Z

@KostyaSha are you sure that isn't an issue for you anyway with either master or the base version this PR was created against? there is no change in this PR that should create that issue for you

KostyaSha · 2016-12-11T20:15:55Z

i'm checking around...

KostyaSha · 2016-12-11T20:20:35Z

revert to 1.22.1 works.

stephenc · 2016-12-11T20:37:54Z

Well this PR is off of 1.22.4 so I claim your issue is outside of the scope of this PR

KostyaSha · 2016-12-11T20:40:07Z

1.22.4 also works.

KostyaSha · 2016-12-11T20:49:11Z

Caused by: 
java.lang.NoSuchMethodError: com.cloudbees.plugins.credentials.common.StandardListBoxModel.includeEmptyValue()Lcom/cloudbees/plugins/credentials/common/AbstractIdCredentialsListBoxModel;
	at org.jenkinsci.plugins.github.config.GitHubServerConfig$DescriptorImpl.doFillCredentialsIdItems(GitHubServerConfig.java:291)

stephenc · 2016-12-11T20:55:44Z

Have you upgraded credentials?

KostyaSha · 2016-12-11T20:55:59Z

src/main/java/org/jenkinsci/plugins/github/extension/GHEventsSubscriber.java

+                    Method isApplicable = clazz.getDeclaredMethod("isApplicable", Item.class);
+                    if (isApplicable.getDeclaringClass() != GHEventsSubscriber.class) {
+                        // ok this is the first method we have found that could be an override
+                        // if somebody overrode an inherited method with and `abstract` then we don't have the method


override?

past tense of override is overrode

KostyaSha · 2016-12-11T20:57:21Z

Have you upgraded credentials?

seems no, i'm using hpi:run from my plugin and comparing on working variant. So yes, maybe caused by super latest requirement.

KostyaSha · 2016-12-11T21:02:48Z

hpi from pr build seems working, looks like something strange in your PR head even after rebase

KostyaSha · 2016-12-11T21:21:21Z

Workflow messed all plugins to work with Job that is not suitable in many cases. Now github-plugin should extend to Item. I see no any methods that could be useful for Item. Why top-level hierarchy item should deal with gh?? If this PR about BuildableItem, then it should be BuildableItem . And even in BuildableItem i see no any useful types for storing something useful. There is no properties, actions. Only methods to build and display. So type expansion is under huge question.

AbstractTopLevelItem/AbstractItem at least has actions!

KostyaSha · 2016-12-11T21:24:09Z

I reviewed all methods and types. Maybe i see bit more than core provides. I can switch to core sources.

stephenc · 2016-12-11T21:24:29Z

@KostyaSha this is not about workflow, but about removing the assumption that the only thing that would want to know about events are jobs and that the only things to trigger are Jobs.

For example https://github.com/jenkinsci/cloudbees-folder-plugin/blob/41f1e2d63f8e4ef82bb845a01fc81ea5e427f465/src/main/java/com/cloudbees/hudson/plugins/folder/computed/ComputedFolder.java#L97 is a BuildableItem and may want to listen for events from the event handler in order to generate items based on the changes.

stephenc · 2016-12-11T21:26:15Z

IMHO we are better allowing the events to be picked up by any generic Item (as that way we can at least mix-in the Item interface rather than relying on the abstract base class.

The trigger should work on anything that is a BuildableItem because that is where a trigger makes sense

KostyaSha · 2016-12-11T21:29:04Z

Ideally should be TrigerableItem :/ Though if item wants and subscriber can get. Then ok. Penalty is that now hook iteration will deal with Jenkins.getAllItems() that literally is all items.

KostyaSha · 2016-12-11T21:30:59Z

I just fear another brainf.. like Parametrized.JobMixIn that doesn't allow to mock tests because methods needs crazy generics and etc.

abstract method (Item) picks for old binary child abstract(Job) calls, right?

KostyaSha

Verified manually that hpi doesn't break binary compatibility, though it impossible to run mvn tests against fresh github-plugin automatically

stephenc · 2016-12-11T21:40:59Z

Thanks for merging. Any estimate on when a release can be cut?

KostyaSha · 2016-12-11T21:42:22Z

Fixing different since. Maybe it possible add empty checkstyle in parent pom that will have single @since {TODO/FIXME//} on fail build level?

KostyaSha · 2016-12-11T22:06:02Z

released.

[FIXED JENKINS-39533] Make GitHub plugin BuildableItem aware

c9cf37d

lanwen approved these changes Nov 8, 2016

View reviewed changes

oleg-nenashev requested changes Nov 8, 2016

View reviewed changes

stephenc mentioned this pull request Nov 8, 2016

[JENKINS-39590] Switch to GitHub.parseEventPayload for event parsing #155

Merged

Address code review comments from Oleg

fb03923

KostyaSha requested changes Dec 8, 2016

View reviewed changes

stephenc changed the title ~~[FIXED JENKINS-39533] Make GitHub plugin BuildableItem aware~~ [FIXED JENKINS-39553] Make GitHub plugin BuildableItem aware Dec 8, 2016

not actually deprecated

4088eb3

stephenc force-pushed the jenkins-39553 branch from d8113e2 to 4088eb3 Compare December 8, 2016 21:48

KostyaSha reviewed Dec 8, 2016

View reviewed changes

Address review comments

7608d99

oleg-nenashev approved these changes Dec 9, 2016

View reviewed changes

KostyaSha reviewed Dec 11, 2016

View reviewed changes

KostyaSha approved these changes Dec 11, 2016

View reviewed changes

KostyaSha merged commit bd2e945 into jenkinsci:master Dec 11, 2016

stephenc deleted the jenkins-39553 branch December 12, 2016 09:03

[FIXED JENKINS-39553] Make GitHub plugin BuildableItem aware #153

[FIXED JENKINS-39553] Make GitHub plugin BuildableItem aware #153

Uh oh!

Conversation

stephenc commented Nov 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Nov 7, 2016

Uh oh!

oleg-nenashev left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephenc commented Dec 7, 2016

Uh oh!

KostyaSha commented Dec 7, 2016

Uh oh!

stephenc commented Dec 7, 2016

Uh oh!

KostyaSha commented Dec 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephenc commented Dec 8, 2016

Uh oh!

KostyaSha left a comment

Choose a reason for hiding this comment

Uh oh!

KostyaSha commented Dec 8, 2016

Uh oh!

stephenc commented Dec 8, 2016

Uh oh!

stephenc commented Dec 8, 2016

Uh oh!

KostyaSha commented Dec 8, 2016

Uh oh!

stephenc commented Dec 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oleg-nenashev left a comment

Choose a reason for hiding this comment

Uh oh!

stephenc commented Dec 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephenc commented Dec 9, 2016

Uh oh!

KostyaSha commented Dec 11, 2016

stephenc commented Nov 7, 2016 •

edited

Loading

oleg-nenashev left a comment •

edited

Loading

KostyaSha commented Dec 7, 2016 •

edited

Loading

stephenc commented Dec 9, 2016 •

edited

Loading