search

jenkinsci / gogs-webhook-plugin

Jenkins Gogs Webhook
https://plugins.jenkins.io/gogs-webhook/
MIT License
79 stars 42 forks source link

Jenkins upgrade to 2.289.1 #75

Closed nhojpatrick closed 2 years ago

nhojpatrick commented 2 years ago

JENKINS-67789/PR-75 - Jenkins upgrade to 2.289.1

Upgrade Jenkins to latest versions and plugins so can start working on multibranchPipelineJob support.

See JENKINS-67789.

Checklist

Types of changes

What types of changes does your code introduce? Put an x in the boxes that apply

Further comments

If this is a relatively large or complex change, start the discussion by explaining why you chose the solution you did and what alternatives you considered.

nhojpatrick commented 2 years ago

@jglick please review again. I have switched to inherited value of jenkins-test-harness.version. I have also added added io.jenkins.tools.bom:bom-2.289.x. I've also removed mvnw. I've also remove the catch all error handling.

MarkEWaite commented 2 years ago

@nhojpatrick if you're interested in exploring some other possible simplifications, refer to the commits at https://github.com/MarkEWaite/gogs-webhook-plugin/commits/jenkins-upgrade . They pass the automated tests and the plugin loads into a Jenkins 2.289.1 session with mvn hpi:run. The dependencies are further simplified and the contents of the plugin hpi file are significantly reduced.

They fail the integration tests in a different way than the integration tests fail for me on the master branch. I didn't spend time investigating the integration test failures.

Thanks for adopting a plugin and for taking the time to use the "Contributing to Open Source" workshop document. Let me know if there are improvements that would have helped make that document clearer and easier to use.

sanderv32 commented 2 years ago

@MarkEWaite How much work will it be to get your changes merged in this plugin? I made this plugin years ago but i don't have the time myself to maintain it and would like to have some help to maintain it and extend it.

MarkEWaite commented 2 years ago

On Tue, Feb 22, 2022 at 1:30 AM Alexander @.***> wrote:

@MarkEWaite https://github.com/MarkEWaite How much work will it be to get your changes merged in this plugin? I made this plugin years ago but i don't have the time myself to maintain it and would like to have some help to maintain it and extend it.

I think that the merge is straightforward, though it would be nice if someone performed some end to end exploratory testing to confirm that there were no surprises for the user. Automated tests are passing and the changes seemed low impact to me when I made them.

Message ID: @.***>

nhojpatrick commented 2 years ago

@sanderv32 @MarkEWaite so my branch is now passing, assuming PR does the integration tests. otherwise please tell me what command i need and i'll check locally. I've review your branch and mine, I can do an annotation changes if needed. Some of the items I've added, i only did so both mvn test and mvn hpi:run. I'll go though each difference and see the bare mine pom i need.

MarkEWaite commented 2 years ago

@sanderv32 @MarkEWaite so my branch is now passing, assuming PR does the integration tests. otherwise please tell me what command i need and i'll check locally.

I'm confident that the integration tests are not run from ci.jenkins.io.

When I ran the integration tests (based on instructions in one of the files in the repo), they failed on the master branch (one failure mode) and failed on my copy of your changes (different failure mode). Since they were failing on the master branch, I didn't spend time investigating further.

The command was:

$ mvn clean install -P withIntegrationTest
nhojpatrick commented 2 years ago

So diff is now only import edu.umd.cs.findbugs.annotations.NonNull; vs import javax.annotation.Nonnull;

Marks version has custom methods vs import static com.google.common.base.Preconditions.checkNotNull;

Any preference in using which non null check and checkNotNull?

nhojpatrick commented 2 years ago

So integration tests, i'm getting these two errors, are they what you are getting.

1) Unexpected error creating GOGS repo: Repository creation call did not return the expected value (returned 401)

2) Exception caught during execution of push command -> Source ref refs/heads/master doesn't resolve to any object.

MarkEWaite commented 2 years ago

So diff is now only import edu.umd.cs.findbugs.annotations.NonNull; vs import javax.annotation.Nonnull;

Mark's version has custom methods vs import static com.google.common.base.Preconditions.checkNotNull;

Any preference in using which non null check and checkNotNull?

Very strong preference for edu.umd.cs.findbugs.annotations.NonNull. Rationale is explained in the commit message. JSR-305 annotations were never adopted in Java. They are not maintained. The spotbugs annotations are maintained in the edu.umd.cs.findbugs.annotations package. The JSR-305 annotations have been removed from core and are steadily being removed from plugins.

Relatively strong preference to remove Guava references. Rationale is explained in the commit message. Guava 11 was originally used in Jenkins, but then the Guava API evolved with compatibility expectations that are different than the Jenkins compatibility expectations. Nothing wrong with either project, they were just different compatibility requirements. Guava has been reduced and removed from Jenkins core and from plugins so that we could upgrade to a current version of Guava. Replacing Guava dependencies with something else will reduce the risk of later complexities from Guava.

MarkEWaite commented 2 years ago

So integration tests, i'm getting these two errors, are they what you are getting.

  1. Unexpected error creating GOGS repo: Repository creation call did not return the expected value (returned 401)
  2. Exception caught during execution of push command -> Source ref refs/heads/master doesn't resolve to any object.

I get the following output on this branch (running Java 8u322 on Debian testing with Docker 20.10.11+dfsg1, build dea9396):

Running org.jenkinsci.plugins.gogs.GogsWebHook_IT
Tests run: 2, Failures: 1, Errors: 1, Skipped: 0, Time elapsed: 12.463 sec <<< FAILURE!
smokeTest_build_testBranch(org.jenkinsci.plugins.gogs.GogsWebHook_IT)  Time elapsed: 5.351 sec  <<< FAILURE!
java.lang.AssertionError: Unexpected error creating GOGS repo: Repository creation call did not return the expected value (returned 401)
        at org.junit.Assert.fail(Assert.java:89)
        at org.jenkinsci.plugins.gogs.GogsWebHook_IT.doItTest(GogsWebHook_IT.java:121)
        at org.jenkinsci.plugins.gogs.GogsWebHook_IT.smokeTest_build_testBranch(GogsWebHook_IT.java:84)
...
smokeTest_build_masterBranch(org.jenkinsci.plugins.gogs.GogsWebHook_IT)  Time elapsed: 7.077 sec  <<< ERROR!
org.apache.http.client.HttpResponseException: status code: 403, reason phrase: Forbidden
        at com.offbytwo.jenkins.client.validator.HttpResponseValidator.validateResponse(HttpResponseValidator.java:11)
        at com.offbytwo.jenkins.client.JenkinsHttpClient.post_xml(JenkinsHttpClient.java:342)
        at com.offbytwo.jenkins.JenkinsServer.createJob(JenkinsServer.java:382)
        at com.offbytwo.jenkins.JenkinsServer.createJob(JenkinsServer.java:338)
        at org.jenkinsci.plugins.gogs.GogsWebHook_IT.doItTest(GogsWebHook_IT.java:174)
        at org.jenkinsci.plugins.gogs.GogsWebHook_IT.smokeTest_build_masterBranch(GogsWebHook_IT.java:74)

Results :

Failed tests:
  GogsWebHook_IT.smokeTest_build_testBranch:84->doItTest:121 Unexpected error creating GOGS repo: Repository creation call did not return the expected value (returned 401)

Tests in error:
  GogsWebHook_IT.smokeTest_build_masterBranch:74->doItTest:174 » HttpResponse st...

Tests run: 2, Failures: 1, Errors: 1, Skipped: 0
MarkEWaite commented 2 years ago

So integration tests, i'm getting these two errors, are they what you are getting.

  1. Unexpected error creating GOGS repo: Repository creation call did not return the expected value (returned 401)
  2. Exception caught during execution of push command -> Source ref refs/heads/master doesn't resolve to any object.

I get the following output on the master branch (running Java 8u322 on Debian testing with Docker 20.10.11+dfsg1, build dea9396):

Running org.jenkinsci.plugins.gogs.GogsWebHook_IT
Tests run: 2, Failures: 1, Errors: 1, Skipped: 0, Time elapsed: 12.806 sec <<< FAILURE!
smokeTest_build_testBranch(org.jenkinsci.plugins.gogs.GogsWebHook_IT)  Time elapsed: 5.754 sec  <<< FAILURE!
java.lang.AssertionError: Unexpected error creating GOGS repo: Repository creation call did not return the expected value (returned 401)
        at org.junit.Assert.fail(Assert.java:88)
        at org.jenkinsci.plugins.gogs.GogsWebHook_IT.doItTest(GogsWebHook_IT.java:121)
        at org.jenkinsci.plugins.gogs.GogsWebHook_IT.smokeTest_build_testBranch(GogsWebHook_IT.java:84)

smokeTest_build_masterBranch(org.jenkinsci.plugins.gogs.GogsWebHook_IT)  Time elapsed: 6.906 sec  <<< ERROR!
org.apache.http.client.HttpResponseException: Forbidden
        at com.offbytwo.jenkins.client.validator.HttpResponseValidator.validateResponse(HttpResponseValidator.java:11)
        at com.offbytwo.jenkins.client.JenkinsHttpClient.post_xml(JenkinsHttpClient.java:342)
        at com.offbytwo.jenkins.JenkinsServer.createJob(JenkinsServer.java:382)
        at com.offbytwo.jenkins.JenkinsServer.createJob(JenkinsServer.java:338)
        at org.jenkinsci.plugins.gogs.GogsWebHook_IT.doItTest(GogsWebHook_IT.java:174)
        at org.jenkinsci.plugins.gogs.GogsWebHook_IT.smokeTest_build_masterBranch(GogsWebHook_IT.java:74)

Results :

Failed tests:
  GogsWebHook_IT.smokeTest_build_testBranch:84->doItTest:121 Unexpected error creating GOGS repo: Repository creation call did not return the expected value (returned 401)

Tests in error:
  GogsWebHook_IT.smokeTest_build_masterBranch:74->doItTest:174 » HttpResponse Fo...

Tests run: 2, Failures: 1, Errors: 1, Skipped: 0
nhojpatrick commented 2 years ago

@MarkEWaite I think it's csrf protection related.

1) mvn clean pre-integration-test -P withIntegrationTest 2) jenkins /script execute the following

import hudson.security.csrf.DefaultCrumbIssuer
import jenkins.model.Jenkins
if (!Jenkins.instance.isQuietingDown()) {
    def j = Jenkins.instance
    if (j.getCrumbIssuer() != null) {
        j.setCrumbIssuer(null)
        j.save()
    }
}

3) mvn failsafe:integration-test -P withIntegrationTest

I appear to get further on my jenkins-update branch. Well the stacktrace appears to show move lines in org.jenkinsci.plugins.gogs.GogsWebHook_IT.doItTest have been executed.

But I've had nighmare with csrf recently so will need help.

MarkEWaite commented 2 years ago

But I've had nighmare with csrf recently so will need help.

Unfortunately, my experiences with automated end to end tests has lead me to use a completely different technique. I maintain a Jenkins instance with configuration as code. I define jobs in that Jenkins instance that I can use to interactively check status and perform experiments while relying on my other production servers (like my Gitea server).

I've not had much success with automated end to end tests. I'm truly impressed by the techniques used in this repository, but I'm not ready to spend time debugging those types of tests.

jglick commented 2 years ago

https://github.com/jenkinsci/gogs-webhook-plugin/blob/master/about_integration_tests.md#integration-test-internals sounds rather complicated. Consider using RealJenkinsRule plus Testcontainers for something that is more IDE-friendly (not reliant on Maven). Automated end-to-end tests are a real pain when dealing with SaaS or licensed products (you have to pick between WireMock or requiring authentication to run tests), but since Gogs is OSS it should be fine.

If tests fail when CSRF protection is enabled, the test might be wrong, but the code being tested might also be wrong. Do not disable CSRF protection as a workaround.

nhojpatrick commented 2 years ago

@jglick this is my 1st jenkins integration tests, and until @MarkEWaite highlighted they where failed I didn't know they event existed. I've just been doing rm -rf work/plugins && mvn clean install && mvn hpi:run. I'm only trying to upgrade so I can submit a pr so that this plugin supports gogs calling back to a pipeline multibranch job.

Do you know another plugin, maybe a bitbucket or github, where it stands up a git repo server. As the tests does a commit and pushes it to gogs, which then fires the webhook, and the the tests detects the job got triggered. That is my current working assumption regarding these IT's.

jglick commented 2 years ago

Do you know another plugin, maybe a bitbucket or github, where it stands up a git repo server.

Neither of those could, at least not easily, due to the proprietary nature (though it seems you can make a BB Server test instance with a trial license or something). They both use WireMock.

I do not know anything about this com.offbytwo.jenkins.client.JenkinsHttpClient. Normally plugins make HTTP requests to the Jenkins controller in functional tests using JenkinsRule.WebClient.

nhojpatrick commented 2 years ago

So I've been able to fix 1 of the GogsWebHook_IT tests by simply changing; jobAtIntitalState.build();

to; jobAtIntitalState.build(true);

sanderv32 commented 2 years ago

To be honest, the integration tests where made because in the beginning PR's where submitted which broke the plugin and the Jenkins CI lacked testing at that moment.

nhojpatrick commented 2 years ago

So i'm moving forward and backwards, now i need to move ~/.ssh and ~/.gitconfig_global otherwise i get other errors. One example is i've a global init.defaultbranch=main but test assumes it's called master.

MarkEWaite commented 2 years ago

So i'm moving forward and backwards, now i need to move ~/.ssh and ~/.gitconfig_global otherwise i get other errors. One example is i've a global init.defaultbranch=main but test assumes it's called master.

That's the same complication I've had to manage in the git client plugin automated tests and the git plugin automated tests. State that is outside the repository (in this case, the user's git configuration) affects the behavior of the test. On the positive side, that means the test is exploring things that are nearer to real users. On the negative side, it means that the tests need to be extended to handle multiple configurations.

jglick commented 2 years ago

State that is outside the repository (in this case, the user's git configuration) affects the behavior of the test.

Something best handled with Testcontainers. (At least for code run on agents. Currently RealJenkinsRule does not support running the controller in a defined container, but I think this could be added.)

nhojpatrick commented 2 years ago

@jglick @MarkEWaite i think it's all good to be merged. As the change I was actually wanted to make was in the logic so it handles multibranchPipelineJob and triggers sub jobs when that branch is changed.

sanderv32 commented 2 years ago

LGTM also