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-54128] Avoid calling Run.getLogFile #40

Merged
merged 1 commit into from
Jun 15, 2019

Conversation

jglick
Copy link
Member

@jglick jglick commented Jun 3, 2019

JENKINS-54128 for this plugin. @vmassol

@jglick jglick requested review from dwnusbaum and ikedam June 3, 2019 19:07
@vmassol
Copy link

vmassol commented Jun 3, 2019

Awesome, thanks @jglick !

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 to codes looks good to me.
I want comments about target and dependency versions.

<distribution>repo</distribution>
</license>
</licenses>

<properties>
<jenkins.version>2.60.3</jenkins.version>
<jenkins.version>2.121.1</jenkins.version>
Copy link
Member

Choose a reason for hiding this comment

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

Let me see.
getLogReader() is already provided in 2.60.3 and there looks no need to upgrade the target version.
Why do you want this change?

Copy link
Member

Choose a reason for hiding this comment

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

I want to leave target versions as possible to allow users with older Jenkins to install newer versions.

Copy link
Member Author

@jglick jglick Jun 10, 2019

Choose a reason for hiding this comment

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

Why do you want this change?

So that functional tests can verify that it actually works. Plugin versions old enough to be compatible with the 2.60.x line do not include the JEP-210 changes which were the trigger for the issue.

allow users with older Jenkins to install newer versions

Up to you as maintainer. Stats suggest it is pointless as the great majority of users who have even updated to the currently newest plugin version are already on Jenkins 1.121.1 or newer.

Copy link
Member

Choose a reason for hiding this comment

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

I see. Looks good to me.

Notes:

<version>1.60</version>
</dependency>
</dependencies>
</dependencyManagement>
Copy link
Member

Choose a reason for hiding this comment

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

Note: To avoid RequireUpperBoundDeps:

[WARNING] Rule 4: org.apache.maven.plugins.enforcer.RequireUpperBoundDeps failed with message:
Failed while enforcing RequireUpperBoundDeps. The error(s) are [
Require upper bound dependencies error for org.jenkins-ci.plugins:structs:1.14 paths to dependency are:
+-org.jvnet.hudson.plugins:groovy-postbuild:2.5-SNAPSHOT
  +-org.jenkins-ci.plugins:badge:1.2
    +-org.jenkins-ci.plugins:structs:1.14
and
+-org.jvnet.hudson.plugins:groovy-postbuild:2.5-SNAPSHOT
  +-org.jenkins-ci.plugins.workflow:workflow-cps:2.69
    +-org.jenkins-ci.plugins:structs:1.17
and
+-org.jvnet.hudson.plugins:groovy-postbuild:2.5-SNAPSHOT
  +-org.jenkins-ci.plugins.workflow:workflow-step-api:2.20
    +-org.jenkins-ci.plugins:structs:1.18
and
+-org.jvnet.hudson.plugins:groovy-postbuild:2.5-SNAPSHOT
  +-org.jenkins-ci.plugins.workflow:workflow-basic-steps:2.15
    +-org.jenkins-ci.plugins:structs:1.17
and
+-org.jvnet.hudson.plugins:groovy-postbuild:2.5-SNAPSHOT
  +-org.jenkins-ci.plugins:junit:1.20
    +-org.jenkins-ci.plugins:structs:1.2
and
+-org.jvnet.hudson.plugins:groovy-postbuild:2.5-SNAPSHOT
  +-org.jenkins-ci.plugins:badge:1.2
    +-org.jenkins-ci.plugins.workflow:workflow-step-api:2.14
      +-org.jenkins-ci.plugins:structs:1.5
and
+-org.jvnet.hudson.plugins:groovy-postbuild:2.5-SNAPSHOT
  +-org.jenkins-ci.plugins.workflow:workflow-cps:2.69
    +-org.jenkins-ci.plugins.workflow:workflow-api:2.30
      +-org.jenkins-ci.plugins:structs:1.14
and
+-org.jvnet.hudson.plugins:groovy-postbuild:2.5-SNAPSHOT
  +-org.jenkins-ci.plugins.workflow:workflow-cps:2.69
    +-org.jenkins-ci.plugins:scm-api:2.2.6
      +-org.jenkins-ci.plugins:structs:1.9
,
Require upper bound dependencies error for org.jenkins-ci.plugins:script-security:1.39 paths to dependency are:
+-org.jvnet.hudson.plugins:groovy-postbuild:2.5-SNAPSHOT
  +-org.jenkins-ci.plugins:badge:1.2
    +-org.jenkins-ci.plugins:script-security:1.39
and
+-org.jvnet.hudson.plugins:groovy-postbuild:2.5-SNAPSHOT
  +-org.jenkins-ci.plugins:matrix-project:1.12
    +-org.jenkins-ci.plugins:script-security:1.13
and
+-org.jvnet.hudson.plugins:groovy-postbuild:2.5-SNAPSHOT
  +-org.jenkins-ci.plugins.workflow:workflow-cps:2.69
    +-org.jenkins-ci.plugins:script-security:1.58
and
+-org.jvnet.hudson.plugins:groovy-postbuild:2.5-SNAPSHOT
  +-org.jenkins-ci.plugins.workflow:workflow-cps:2.69
    +-org.jenkins-ci.plugins.workflow:workflow-support:3.3
      +-org.jenkins-ci.plugins:script-security:1.39
,
Require upper bound dependencies error for org.jenkins-ci.plugins.workflow:workflow-api:2.30 paths to dependency are:
+-org.jvnet.hudson.plugins:groovy-postbuild:2.5-SNAPSHOT
  +-org.jenkins-ci.plugins.workflow:workflow-cps:2.69
    +-org.jenkins-ci.plugins.workflow:workflow-api:2.30
and
+-org.jvnet.hudson.plugins:groovy-postbuild:2.5-SNAPSHOT
  +-org.jenkins-ci.plugins.workflow:workflow-job:2.32
    +-org.jenkins-ci.plugins.workflow:workflow-api:2.32
and
+-org.jvnet.hudson.plugins:groovy-postbuild:2.5-SNAPSHOT
  +-org.jenkins-ci.plugins.workflow:workflow-basic-steps:2.15
    +-org.jenkins-ci.plugins.workflow:workflow-api:2.33
and
+-org.jvnet.hudson.plugins:groovy-postbuild:2.5-SNAPSHOT
  +-org.jenkins-ci.plugins.workflow:workflow-cps:2.69
    +-org.jenkins-ci.plugins.workflow:workflow-support:3.3
      +-org.jenkins-ci.plugins.workflow:workflow-api:2.30

It would be warned if versions in dependencyManagement is older than actual required ones, and this is enough safe even when updating dependency versions.

Copy link
Member

Choose a reason for hiding this comment

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

But there looks artifacts not warned in RequireUpperBoundDeps.
And the versions looks newer than required ones.
With what policy do you specify these versions?

I want to know the best practice to avoid RequireUpperBoundDeps.

Copy link
Member Author

Choose a reason for hiding this comment

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

With what policy do you specify these versions?

I simply put managed versions of dependencies as new as RequireUpperBoundDeps errors requested, until there no more such errors.

Copy link
Member

Choose a reason for hiding this comment

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

OK, thanks.

<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>script-security</artifactId>
<version>1.60</version>
Copy link
Member

Choose a reason for hiding this comment

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

(This is just a note, not a change request)
Groovy-postbuild depends directly and enormously on script-security, and should depend on script-security explicitly and declare the same version in tests.
It’s removed from dependencies when extracting badge-plugin.
I’ll review dependencies after merging this and #42 .

Copy link
Member

Choose a reason for hiding this comment

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

I created #43.

@ikedam ikedam merged commit 8f48cbe into jenkinsci:master Jun 15, 2019
ikedam added a commit to ikedam/groovy-postbuild-plugin that referenced this pull request Jun 15, 2019
* I fond that doesn't affect plugin dependencies.
@ikedam ikedam mentioned this pull request Jun 15, 2019
@jglick jglick deleted the Run.logFile-JENKINS-54128 branch June 15, 2019 13:04
@ikedam
Copy link
Member

ikedam commented Jun 24, 2019

Released in groovy-postbuild-2.5.
It will be available in the update center in a day.

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

Successfully merging this pull request may close these issues.

5 participants