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

Add people with merge rights #366

Open
erikhakansson opened this issue Apr 11, 2018 · 36 comments
Open

Add people with merge rights #366

erikhakansson opened this issue Apr 11, 2018 · 36 comments

Comments

@erikhakansson
Copy link
Contributor

Nothing has happened with this plugin for a long time despite some issues. If @jan-molak is too busy, maybe more people should be added to the project to offload?

@jan-molak
Copy link
Member

Help would be greatly appreciated, let me know if anyone has capacity to become the member of the "core team" :-)

@erikhakansson
Copy link
Contributor Author

I would love to, but I'm not sure I'm really qualified nor that I have much more time than you.

However, since it's in my and my company's personal interest to have some of the outstanding PR:s merged, I'd certainly be willing to have a look at them.

Specifially my own PR #293 as well as #365

@erikhakansson
Copy link
Contributor Author

erikhakansson commented Apr 16, 2018

Maybe you should add something to the readme and/or the plugin wiki that you're looking for contributors?

@jan-molak
Copy link
Member

Thanks for offering your help, @erikhakansson! It would be awesome if you could help out with getting #365 to a mergeable state as that issue seems to affect a bunch of people.

@erikhakansson
Copy link
Contributor Author

I'll start looking at that.
However, I had to add proxy support to make tests work behind proxy, so I added a PR for that (#368).

However, not all tests go through and they shouldn't be related to the proxy stuff, so I wonder if the tests are working for you in the master branch?

@dcendents
Copy link
Contributor

The master branch is also broken. The problem is that the tests will pull the latest version of the plugins that the build-monitor plugins can integrate with (using the install manager) but those plugins are not compatible anymore with the jenkins core that is being used.

In my opinion this is two different problems:

  1. The tests should use the versions as declared in the pom (and make sure these exact plugin versions are installed and not the latest).
  2. The jenkins core version used should be maintained to a reasonable baseline.

@erikhakansson
Copy link
Contributor Author

@dcendents ok, I see!
Would bumping to the latest Jenkins version break compatibility with older versions? I.e. would it be better to bump to a newer, but not the latest version?

@dcendents
Copy link
Contributor

Look at this: https://wiki.jenkins.io/display/JENKINS/Choosing+Jenkins+version+to+build+against

I suppose the best thing would be to keep a low version (2.19.4), test against that version but also find a way to test against the latest LTS as well (maven profiles?).

@erikhakansson
Copy link
Contributor Author

Okay thanks.
I'll do it in two steps then.
Currently I'm working on getting the current master to work with the old version of Jenkins by defining specific plugin versions.

After that I'll see what has to be done to run on later Jenkins versions

@dcendents
Copy link
Contributor

The more I think about it maybe we should let the install manager as it is and install the latest version of the plugins.

Since 2.46, jenkins have introduced different update sites to make sure we get only plugins compatible with a given core version.

e.g.:

So it might be a good idea to keep the current behavior as it would test what the users experience. It means the tests might break when a dependent plugin is updated but at least we are testing the reality.

So we could update the core version to 2.46.3 and find a way to test every LTS release since then. If a new LTS release breaks the build, we either try to be backwards compatible or we increase the core version and it means only people using the latest LTS will get the new features. People using and older version will still be able to install an older version of the build monitor that was compatible with that specific LTS version.

e.g.:

  • BuildMonitor 2.0 -> Jenkins 2.46+
  • BuildMonitor 2.1 -> Jenkins 2.107+

If I use jenkins 2.89.3, the update site would let me install version 2.0 which is fine for me. To get new features I need to update jenkins to 2.107.1

@erikhakansson
Copy link
Contributor Author

Yeah that sound's like a plan. And I was almost done with fixing the master branch the old way 😏

@erikhakansson
Copy link
Contributor Author

I'll still do it twofold, though, although the steps will be

  1. Bump Jenkins version to 2.46.3 and fix everything
  2. Implement future-testing

@erikhakansson
Copy link
Contributor Author

Status update:

I've bumped the Jenkins version to 2.46.3, the org.jenkins-ci.plugins:plugin parent to 3.8 and the jenkins-test-harness to 2.38.

The bumped parent pom includes a lot of new Maven Enforcer rules and findbugs rules that I had to solve, so after wrestling with conflicting dependencies, and bumping some here and there and simply overriding some rules I've gotten everything to compile.

build-monitor-plugin builds without issue now, while pretty much every test in build-monitor-acceptance fails. So the next step is to solve that.

I haven't merged in #365 yet, though, as I want as much as possible working before I do that.

@erikhakansson
Copy link
Contributor Author

Another status update:
I've gotten 6 of 11 tests to work. It has mostly been updating selenium tests that has been necessary.
I've also had to wrestle some with getting the ITs to work on a Windows/WSL environment but it works now.

I still haven't run into any issues regarding #364 but I think one test might have that issue (Badges) so I'll leave that for last.

Hopefully I'll have a PR soon. I hope to be able to work on it the coming evenings. It would be great if both @jan-molak and @dcendents could have a look at it then.

@erikhakansson
Copy link
Contributor Author

@jan-molak and @dcendents please have a look at #369.

Note that I have ONLY bumped the Jenkins version in that. I haven't merged the #365 PR, nor have I made forward-testing possible. I think that should be a combined second step after this is merged.

@jan-molak
Copy link
Member

@erikhakansson sounds like a plan! I've merged #369, happy to help with the subsequent PRs.

Jan

@dcendents
Copy link
Contributor

Thanks @erikhakansson , I was actually logging in to check the work like you asked, I'm glad @jan-molak had time to do it and merge it.

@erikhakansson
Copy link
Contributor Author

Thanks guys!

I'm on to step two, which is "future"-testing with the latest LTS. There were some changes in AbstractBuild (on the Jenkins side) that required some additional mocks for the unit tests to work, but it was easily solved with PowerMockito (to maintain class compatibility between 2.46 and newer).

The integration tests are going fine, except the Badges one. That badges don't work was expected since that's pretty much the core of #364.

However, I'm having some difficulties applying the fix while still retaining compatibility with older Jenkins versions. But I'm working on it!

@erikhakansson
Copy link
Contributor Author

erikhakansson commented Apr 27, 2018

I've solved compatibility with both 2.46.3 and 2.107.2, but I managed to broke a couple of tests in the process so I'm working on solving them.

However, I'm having some difficulties in automating testing both. When manually setting ${jenkins.version} it works. When adding executions to run both in different phases, using one ${jenkins.version} property and one ${jenkins.lts.version} property I get weird errors in Jenkins. I think it's related to the Jenkins Test Harness forcing the ${jenkins.version} on some dependencies.

For now I added a profile -Plts-test that changes ${jenkins.version} to 2.107.2 and that way it works, but it would be far prettier to have it as part of the normal mvn clean verify process.

@dcendents
Copy link
Contributor

I was thinking about the problem and I suppose it cannot be done easily with a single maven module. To run both sets of tests I think you will need to split them in different modules.

  • build-monitor: common parent
    • build-monitor-plugin: the plugin
    • build-monitor-acceptance: Becomes a parent, can define common dependencies here
      • build-monitor-acceptance-base: Java tests against base version, will also produce a test-jar
      • build-monitor-acceptance-latest: Run same tests (from build-monitor-acceptance-base test-jar) against latest lts

It shouldn't break the builds with cloudbees' jenkins.

Otherwise we could start different jenkins instances (using docker) and configure a selenium grid to run the tests against both of them at the same time. But it might be harder to configure that in a free publicly available build server (cloudbees, travis, etc).

@dcendents
Copy link
Contributor

Well it seems the "standard" way of testing plugins against other jenkins version is to use the plugin-compat-tester: https://github.com/jenkinsci/plugin-compat-tester

The documentation is a bit poor but they have an example here under Testing plugins against Jenkins 2.102 and above: https://jenkins.io/blog/2018/01/13/jep-200/

This means the tests would need to be run manually though.

@erikhakansson
Copy link
Contributor Author

Unless I misunderstand, plugin-compat-tester isn't really for automated testing, right?

I went with your first suggestion, splitting upp acceptance into two modules.

PR #370 is waiting for review 😄 @jan-molak I added some comments for you where I was unsure of stuff.

@erikhakansson
Copy link
Contributor Author

#370 is updated.

I noticed that an old version of maven-enforcer-plugin was hardcoded in one of the submodules so I removed that to enable inheritance from the parent pom. I haven't had any issues locally but it could be related to your build troubles.

@erikhakansson
Copy link
Contributor Author

Ping @jan-molak had any time to check the PR?

I'm running it locally at work here (Well actually, I'm running #293 , but I've rebased that on #370) and it works fine.

@erikhakansson
Copy link
Contributor Author

Also, I have a suggestion with dealing with the backlog of PRs. Since a lot of them, if not most, need to be rebased on the Jenkins 2 functionality, you could close all of them with a message like "Major changes have been made to Build Monitor Plugin. Therefore I'm closing all older PR's. Please rebase you PR against master and reopen pull request if still relevant". Or something.

@dcendents
Copy link
Contributor

#370 builds fine for me as well. I had some small improvements I wanted to submit, I'll rebase on that and add some acceptance tests.

@jan-molak
Copy link
Member

jan-molak commented May 5, 2018

It's looking good, thanks for your help and support, gents! I've just merged in #370 and it's building on Jenkins as we speak.

@erikhakansson
Copy link
Contributor Author

erikhakansson commented May 6, 2018

Thanks!

I've added #371 to hopefully solve the issue you had.

@jan-molak
Copy link
Member

Thanks! The build has just succeeded on Jenkins, so you can take the hpi for a spin if you'd like.

@erikhakansson
Copy link
Contributor Author

Great news! Thanks! 😄

I've also rebased #293 which was my initial motivation for getting involved 😄

I'd be very happy if you could have a look at that!

@erikhakansson
Copy link
Contributor Author

Any news, @jan-molak ?

Both on a release of the current state and on #293

@jan-molak
Copy link
Member

Hi @erikhakansson! I've released the last build and should have some time over the next week or two to look into #293. Thanks for your efforts!

@erikhakansson
Copy link
Contributor Author

Great, thanks!

@erikhakansson
Copy link
Contributor Author

Had time yet? 😄

@jan-molak
Copy link
Member

Hey guys, apologies, I've been snowed under.

I'll set up a gitter channel for us this evening so that we can discuss giving people the merge rights and so on. Thanks again for offering your help and support!

JAn

@erikhakansson
Copy link
Contributor Author

That sounds great!

I think that some merge and release rights would be the best course of action to avoid stagnation!

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

No branches or pull requests

3 participants