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

[JENKINS-35014] Migrate to new parent POM #10

Merged
merged 2 commits into from Jul 1, 2016

Conversation

Projects
None yet
7 participants
@andresrc
Copy link

commented May 23, 2016

See JENKINS-35014.

  • Migrate to parent pom 2.9
  • Javadoc fixes in Java 8.
  • Findbugs fixed.
  • PCT checked against 1.651.2 and 2.6

@reviewbybees

pom.xml Outdated
<maven.compiler.source>1.7</maven.compiler.source>
<maven.compiler.target>1.7</maven.compiler.target>
<jenkins.version>1.554.3</jenkins.version>
<java.level>7</java.level>

This comment has been minimized.

Copy link
@armfergom

armfergom May 23, 2016

Shouldn't this be <java.level>6</java.level>?

if (store.supports(clazz)) {
return store;
final Jenkins jenkins = Jenkins.getInstance();
if (jenkins != null) {

This comment has been minimized.

Copy link
@armfergom

armfergom May 23, 2016

Question, not correction: I thought we could assume that Jenkins.getInstance() should never be null in a plugin: jenkinsci/jenkins#2094

Or I misunderstood?

This comment has been minimized.

Copy link
@stephenc

stephenc May 23, 2016

Member

Baseline is 1.554.3 which is @CheckForNull so findbugs will complain

This comment has been minimized.

Copy link
@armfergom

armfergom May 23, 2016

I think I did not explain myself correctly, sorry. My question was, it could also be ignored, correct?(@SuppressFBWarnings)

This comment has been minimized.

Copy link
@andresrc

andresrc May 23, 2016

Author

@armfergom yes but I prefer to avoid ignores if a simple fix as available, as ignores may remain there forever...

This comment has been minimized.

Copy link
@armfergom

armfergom May 23, 2016

Thanks @andresrc , just wanted to be sure that where I did use ignores, it was ok to do it.

This comment has been minimized.

Copy link
@stephenc

stephenc May 23, 2016

Member

When we fix core to have getInstance() be @Nonnull after having banned Jenkins from being loaded over remoting then this will pop up as a findbugs error to fix, whereas the @SuppressFBWarnings would never show and thus increase technical debt... never mind that @SuppressFBWarnings may also disguise additional bugs in the method

This comment has been minimized.

Copy link
@jglick

jglick Jun 17, 2016

Member

tl;dr: use getInstance if in your current baseline it is @Nonnull, else use getActiveInstance if it exists, else use getInstance with a null check.

All assuming that you are in the context of code which should assume Jenkins is up and running. There are a few cases where you really do want to do something other than throw an exception when it is not.

if (store.supports(clazz)) {
return store;
final Jenkins jenkins = Jenkins.getInstance();
if (jenkins != null) {

This comment has been minimized.

Copy link
@armfergom

armfergom May 23, 2016

Same question as above :)

@reviewbybees

This comment has been minimized.

Copy link

commented Jun 16, 2016

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.

Andres Rodriguez
@@ -24,21 +24,21 @@
</scm>

<properties>
<maven.compiler.source>1.7</maven.compiler.source>
<maven.compiler.target>1.7</maven.compiler.target>
<jenkins.version>1.615</jenkins.version>

This comment has been minimized.

Copy link
@andresrc

andresrc Jun 17, 2016

Author

The code uses Java 7 features, so baseline bumped to the first version using Java 7.

This comment has been minimized.

Copy link
@jglick

jglick Jun 17, 2016

Member

as elsewhere, would suggest the next LTS, 1.625.x

@armfergom

This comment has been minimized.

Copy link

commented Jun 17, 2016

🐝

return store;
final Jenkins jenkins = Jenkins.getInstance();
if (jenkins != null) {
for (IdStore store : jenkins.getExtensionList(IdStore.class)) {

This comment has been minimized.

Copy link
@jglick

jglick Jun 17, 2016

Member

…but if you are in a version that contains ExtensionList.lookup, use it instead.

@@ -20,7 +20,7 @@

/**
* Stores ids for folders as a {@link FolderIdProperty}
* @deprecated {@see PersistenceRootIdStore}
* @deprecated Use {@link org.jenkinsci.plugins.uniqueid.implv2.PersistenceRootIdStore}

This comment has been minimized.

Copy link
@jglick

jglick Jun 17, 2016

Member

Or can just import it.

return store;
final Jenkins jenkins = Jenkins.getInstance();
if (jenkins != null) {
for (LegacyIdStore store : jenkins.getExtensionList(LegacyIdStore.class)) {

This comment has been minimized.

Copy link
@jglick

jglick Jun 17, 2016

Member

ditto

@jglick

This comment has been minimized.

Copy link
Member

commented Jun 17, 2016

🐝

@kwhetstone

This comment has been minimized.

Copy link

commented Jun 30, 2016

🐝 Are we ready to merge?

@jtnord

This comment has been minimized.

Copy link
Member

commented Jul 1, 2016

🐝 / 👍

@jtnord jtnord merged commit eed1a80 into jenkinsci:master Jul 1, 2016

1 check passed

Jenkins This pull request looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.