jenkinsci / build-timeout-plugin

Jenkins build-timeout plugin
https://plugins.jenkins.io/build-timeout/
31 stars 80 forks source link

[JENKINS-34846] Migrate to 2.9 plugin pom #53

Closed armfergom closed 8 years ago

armfergom commented 8 years ago

@reviewbybees

ghost commented 8 years ago

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.

armfergom commented 8 years ago

Reopening to retrigger CI build. This time with-Dconcurrency=4 instead of -Dconcurrency=1C

armfergom commented 8 years ago

mvn clean install builds ok. CI job is configured to mark build as unstable if there are findbugs errors though. That's why this PR has failed checks.

Fixing findbugs errors were out of the scope of this PR, but if it needs to be done to get it merged I'll revisit it.

ikedam commented 8 years ago

The change looks good. (Of course, you need to fix findbug errors) But I don't like to upgrade the target Jenkins version unless it is actually necessary for features of the plugin.

I want to know more contexts of this request (that is, why you want this change). Do you plan to migrate all plugins to parent pom 2.x?

armfergom commented 8 years ago

@ikedam There is no parent pom since 1.646 onwards so the PCT will fail for Jenkins versions above that. Also, it is just an easy win and adds some benefits that you can see here.

I have bumped the baseline version because of incompatibilities with the jenkins test harness, but I can work around it if you prefer to keep the same baseline. Will also fix Findbugs issues.

And no, I am not planing to migrate all plugins, just a few :)

armfergom commented 8 years ago

@ikedam Addressed your comments. Given that you would like to keep 1.466 baseline, the jenkins test harness to use cannot be 2.6. It has to be 1.466 too. Also, sisu-guice is a banned dependency for newer versions of the core, but it is required for 1.466 baseline, so I cannot simply exclude it. For that reason, I've had to override the maven-enforcer-plugin configuration so that sisu-guice is not banned anymore (keeping the other declared banned dependencies in the core).

As for Findbugs errors, when keeping 1.466 baseline, no Findbugs errors are detected.

ikedam commented 8 years ago

Thanks a lot! Looks perfect to me.

And no, I am not planing to migrate all plugins, just a few :)

I hesitated to migrate to the new parent pom as I thouhgt I can't do that without migrating the target Jenkins version (Especially I misunderstood that I have to use jenkins-test-harness 2.x for the new parent pom). You showed me that plugins with older target Jenkins versions can migrate to the new parent pom.m, thanks!

Why don't we have all plugins migrate to the new parent pom? (e.g. announcing on jenkinsci-dev and ask developers to update their plugins)

armfergom commented 8 years ago

@ikedam The thing is, the new parent pom brings the jenkins-test-harness dependency with it. Specifically, version 2.9 of the parent pom brings jenkins-test-harness version 2.6. The problem was that newer versions of the jenkins-test-harness do not work well with older baselines. But fortunately, the new parent pom allows us to override the jenkins-test-harness version to use, so I just stuck with 1.466.

As for migrating (or asking to migrate) all plugins, I don't think it is critical at the moment, rather a nice to have. In any case, I think there is already a thread (here) where this is discussed.

ikedam commented 8 years ago

Oops, the link to google groups doesn't work. (it looks link to the search cache) Can I have the subject of the thread?

armfergom commented 8 years ago

The subject was "[PROPOSAL] New Parent POM for Jenkins Plugins"

ikedam commented 8 years ago

I got it. Thanks. I agree that was the announce. I'll start my other plugins to migrate to the new parent pom.

andresrc commented 8 years ago

:bee:

kwhetstone commented 8 years ago

:bee: Looks good