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

Update dependencies and docs for 4.1 release #797

Merged
merged 15 commits into from
Dec 19, 2019

Conversation

MarkEWaite
Copy link
Contributor

@MarkEWaite MarkEWaite commented Dec 16, 2019

Update docs and dependencies

Update the dependencies and the documentation page as preparation for eventual release of git plugin 4.1.

Checklist

  • I have read the CONTRIBUTING doc
  • Unit tests pass locally with my changes
  • I have added documentation as necessary
  • No Javadoc warnings were introduced with my changes
  • No spotbugs warnings were introduced with my changes
  • Any dependent changes have been merged and published in upstream modules (like git-client-plugin)

Types of changes

  • Dependency or infrastructure update

@MarkEWaite
Copy link
Contributor Author

MarkEWaite commented Dec 16, 2019

@slonopotamus I would also like to update to parent pom 3.54, but when I attempt that, it fails to compile. There seems to be a signature change in the jenkins test harness that I don't understand how to resolve.

[INFO] -------------------------------------------------------------
[ERROR] COMPILATION ERROR :
[INFO] -------------------------------------------------------------
[ERROR] /home/mwaite/git/jenkins/git-plugin/src/test/java/org/jenkinsci/plugins/gittagmessage/AbstractGitTagMessageExtensionTest.java:[161,26] no suitable method found for buildAndAssertSuccess(J)
    method org.jvnet.hudson.test.JenkinsRule.<J,R>buildAndAssertSuccess(J) is not applicable
      (inference variable R has incompatible bounds
        equality constraints: R
        upper bounds: hudson.model.Run<J,R>,hudson.model.Queue.Executable)
    method org.jvnet.hudson.test.JenkinsRule.buildAndAssertSuccess(hudson.model.FreeStyleProject) is not applicable
      (argument mismatch; J cannot be converted to hudson.model.FreeStyleProject)
[INFO] 1 error
[INFO] -------------------------------------------------------------
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  17.207 s
[INFO] Finished at: 2019-12-16T06:12:40-07:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:testCompile (default-testCompile) on project git: Compilation failure
[ERROR] /home/mwaite/git/jenkins/git-plugin/src/test/java/org/jenkinsci/plugins/gittagmessage/AbstractGitTagMessageExtensionTest.java:[161,26] no suitable method found for buildAndAssertSuccess(J)
[ERROR]     method org.jvnet.hudson.test.JenkinsRule.<J,R>buildAndAssertSuccess(J) is not applicable
[ERROR]       (inference variable R has incompatible bounds
[ERROR]         equality constraints: R
[ERROR]         upper bounds: hudson.model.Run<J,R>,hudson.model.Queue.Executable)
[ERROR]     method org.jvnet.hudson.test.JenkinsRule.buildAndAssertSuccess(hudson.model.FreeStyleProject) is not applicable
[ERROR]       (argument mismatch; J cannot be converted to hudson.model.FreeStyleProject)

I believe that was a change you made to the Jenkins test harness. Can you help me understand what needs to change in the plugin to adapt to the harness change?

@slonopotamus
Copy link
Contributor

slonopotamus commented Dec 16, 2019

@MarkEWaite Whoops. Didn't think that this method could be proxied through other generics :(

Here's a patch that fixes AbstractGitTagMessageExtensionTest. If you want a proper PR from it, it'll take some time.

For the reference, compilation failure is caused by jenkinsci/jenkins-test-harness#167.

@slonopotamus
Copy link
Contributor

slonopotamus commented Dec 16, 2019

See #798, it fixes AbstractGitSCMSourceTest.when_commits_added_during_discovery_we_do_not_crash failure when run against newer JTH. I dunno whether you'd like to incorporate it into this PR or merge standalone or create a new PR that will fix all things related to upgrade of parent pom to 3.54, I'm OK with any route.

@slonopotamus
Copy link
Contributor

slonopotamus commented Dec 16, 2019

I suspect that GitTagActionTest was broken even before this PR. But because assertion fails in the different thread, older hamcrest didn't properly mark test as failed.

Hmmm... No, it is working on master branch.

@MarkEWaite
Copy link
Contributor Author

The GitTagActionTest has a race condition that is sometimes visible. I created the test while investigating SECURITY-1095. If it continues to be unreliable, I am very willing to delete the test.

The GitTagAction is a legacy operation in the plugin. It creates a tag in a workspace after the build has completed. The assumption is that the workspace exists after the build completes. That assumption is not valid with ephemeral agents. It does not push the tag anywhere, so the only repository which contains the tag is the repository inside the workspace. Without pushing the tag anywhere, no other users will see the tag.

GitTagAction has no viable use case as far as I can tell. It remains in the plugin because I don't want to spend the time or energy to delete the capability, document its deletion, and explain why the use case is not relevant. It is easier to leave it as idle code than to remove it.

You can read the diffs to see the types of horrors I created while trying to assure that I did not break the GitTagAction by adding the RequirePOST annotation.

Upgrade to more recent parent pom will require that the test dependency
on hamcrest is declared explicitly.
@MarkEWaite MarkEWaite added the documentation Improvements or additions to documentation label Dec 17, 2019
@@ -50,6 +50,8 @@ public CleanBeforeCheckoutTrait() {

/**
* Stapler constructor.
*
* @param extension the option to clean subdirectories which contain git repositories.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@darxriggs I'm open to a better way to describe this parameter and the same parameter on the other trait.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you have a look at the other traits they more or less only link to the type of the parameter. Maybe keep it like this and not duplicate the description of the extension or come up with another description for it.

@MarkEWaite MarkEWaite force-pushed the update-dependencies branch 2 times, most recently from 089db3c to afb6cca Compare December 18, 2019 23:41
Unclear why the test is failing on ci.jenkins.io when it regularly
succeeds in my Windows environments.  Test is not valuable enough to
justify a long research project to identify why it behaves differently
on ci.jenkins.io than on other Windows computers.

Test runs on Linux.  Test coverage is measured on Linux.
@MarkEWaite MarkEWaite merged commit f25db72 into jenkinsci:master Dec 19, 2019
@MarkEWaite MarkEWaite deleted the update-dependencies branch December 19, 2019 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants