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

Modernize plugin #64

Merged
merged 3 commits into from
Oct 2, 2017
Merged

Modernize plugin #64

merged 3 commits into from
Oct 2, 2017

Conversation

daniel-beck
Copy link
Member

@daniel-beck daniel-beck commented Sep 28, 2017


Tests still pass, otherwise I'm not confident I didn't mess up some of the internals. Review is needed.

- Current parent POM
- Update dependency to something that doesn't bring a dozen implied dependencies
- Corresponding Java level update
- Strip redundant parts from plugin name and capitalize it
@daniel-beck daniel-beck requested a review from ikedam September 28, 2017 12:36
@daniel-beck
Copy link
Member Author

If you're concerned about the core dependency increase:
http://stats.jenkins.io/pluginversions/build-timeout.html
1.18 is 10 months old (plenty of time to update) and 100% of its users are on that core version or newer (96% of all users of all versions of the plugin are).

TBH if it were my plugin, I'd go with a 2.x release, but the main point here is to get rid of the unused implied dependencies to detached plugins.

@ikedam
Copy link
Member

ikedam commented Sep 29, 2017

Upgrading the core looks reasonable to me. Targetting old cores cause installing unnecessary plugins as you point, and build-timeout is often installed during the setup wizard.
(At the same time, build-timeout doesn't support pipeline, and may not be useful for many people)

I wonder why ci.jenkins.io doesn't test this request even after merging #65 ...

@ikedam
Copy link
Member

ikedam commented Sep 30, 2017

Test failures caused for

  • The test expects the log line "Test" from FakeBuildStep.
  • Tests on windows always log following output:
Building in workspace C:\Users\ikedam\AppData\Local\Temp\jenkinsTests.tmp\jenkins1377785768819663524test\workspace\test0

So you can fix the test failures by modifying the log line from FakeBuildStep to more appropriate identifiable string.

And we can replace BuildStepWithTimeoutTest#assertLogDoesNotContain to JenkinsRule#assertLogNotContains, which makes test errors more descriptive.

Copy link
Member

@ikedam ikedam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me.
I'd like you to do some additional changes to improve pom.xml and fix tests.

pom.xml Outdated
<forkCount>1</forkCount>
</configuration>
</plugin>
<plugin>
<artifactId>maven-enforcer-plugin</artifactId>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove also configurations for maven-enforcer-plugin.

@daniel-beck daniel-beck self-assigned this Sep 30, 2017
- Change fake build step output to prevent false positive build failures
- Remove maven enforcer plugin rules from pom
@daniel-beck daniel-beck removed their assignment Oct 2, 2017
@daniel-beck
Copy link
Member Author

@ikedam I addressed your review comments.

Copy link
Member

@ikedam ikedam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@ikedam ikedam merged commit 2462161 into jenkinsci:master Oct 2, 2017
@ikedam
Copy link
Member

ikedam commented Oct 8, 2017

Released build-timeout-1.19.
It allows users install the plugin without installing matrix-project so on.

@daniel-beck
Copy link
Member Author

@ikedam Thanks!

@@ -16,7 +16,7 @@

<artifactId>build-timeout</artifactId>
<packaging>hpi</packaging>
<name>Jenkins build timeout plugin</name>
<name>Build Timeout</name>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change of name broke JJB as it queries the jenkins master to get the plugin version, and tries to find it by name. I know its a different project, but wanted to post this here in order to note that this change had more impact than expected in a minor version bump.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relying on the display name that only exists for human use is a terrible idea. Scripts should rely on the immutable artifactId instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants