Upgrade to stapler version 1.246 #2593
Conversation
|
Seems the issue with release availability has not been resolved yet. According to the code changes, I do not believe that it's only one change in three Stapler releases. Please provide a full changelog (ideally with JIRA references if available) |
oleg-nenashev
added
the
needs-more-reviews
|
@oleg-nenashev Yeah not sure what the problem is, hopefully they will appear eventually in the repo. In terms of changes, here are the Stapler PRs (no JIRA ticket though) that got merged post 1.244: |
|
@vivek I'm aware there are many non-PR changes made by @kohsuke. Overall diff: jenkinsci/stapler@stapler-parent-1.243...master I'll try to go through the code ant to provide my feedback by Wednesday. Please let me know if there is anything urgent |
oleg-nenashev
added
the
on-hold
|
@oleg-nenashev Who would know why is Stapler artifacts are not resolved yet?
|
|
@vivek |
|
@oleg-nenashev Right and its not even at http://repo.jenkins-ci.org/public/org/kohsuke/stapler/stapler/. Maybe the repo has moved. @kohsuke Do you know if release went well or it was released to some other repo? |
|
@oleg-nenashev Actually, only the repository changing commit in jenkinsci/stapler@stapler-parent-1.244...stapler-parent-1.246 was without PR. And in jenkinsci/stapler@stapler-parent-1.243...stapler-parent-1.244 only jenkinsci/stapler@9411334 was outside a PR. Still, @vivek's list of PRs is misleading, as this updates from 1.243 to 1.246, not from 1.244. Full list: jenkinsci/stapler#73 |
|
@daniel-beck Thanks, yeah I mistook the from version number, your list is good that is from 1.243. So what happens next to get stapler 1.246 to land in central? It's already released. |
|
@daniel-beck @oleg-nenashev: Stapler 1.246 was released to maven central but was not visible, @kohsuke fixed it. Its visible now. Can Jenkins build be triggered for this PR manually? |
|
@daniel-beck There is one failure but this has been failing since https://jenkins.ci.cloudbees.com/job/core/job/jenkins-core/6375/ and is unrelated. How do we move forward? |
|
@vivek There's a successful PR build, so no problem there from build success POV. Now this needs reviews. @jenkinsci/code-reviewers assemble! |
|
@daniel-beck How long do you stage it for review? Didn't see any comments so far. |
oleg-nenashev
removed
the
on-hold
|
Review is still in my TODO list. |
|
There are some compatibility changes in PRs like jenkinsci/stapler#77 (public classes => final, and several other changes). Putting back on-hold since I'm not comfortable about merging before a wider research |
oleg-nenashev
added
the
on-hold
|
I would rather 🐛 this PR till Stapler has public CI builds. @vivek Is it critical to get it available in the next LTS line? What's the impact if it does not get there? |
|
@oleg-nenashev It is critical as some of the changes/fixes are needed by blueocean. So its really important it gets in to this LTS. Just so that I understand, your objection is for process or there is technical reason you are objecting to? In terms of review, each Stapler PR goes thru review process and has been reviewed/approved. Anyways, all of those PR are already in stapler released version. So if you think there is PR or change that is an issue or can cause instability in Jenkins then we can fix that otherwise I think we should not hold this PR for process reasons unless it violates any Jenkins guidelines. |
|
@vivek Binary compatibility is one of Jenkins requirements. Since there are binary compatibility changes, Stapler maintainers should proof that they do not impact at least OSS plugins. I do not want to block the update since I understand value of BO to the community, but compatibility of existing plugins IMHO is a mandatory requirement unless there is a decision made after the discussion in the ML. |
|
@oleg-nenashev Fair point. Happy to help here. All tests are passing. If there is more testing or review needed, lets get thru that. Its just that the PR is sitting idle so was making sure the concern gets addressed and integration happens if all looks good:) |
|
So there are following issues I see:
So I do not see anything critical in the changes (though I didn't investigate logic much), but I would like to get signoff of other @jenkinsci/code-reviewers since the changes do not qualify as a "safe change" for me. If somebody fixes the binary compatibility issue and creates a working PR builder, you will have my +1 |
|
+1 on the "safe change" comment. There's still quite a few changes there… Normally there would still be time for the next .1 LTS to let it soak anyway, only if the selected base includes it... |
|
@batmat I'm not sure. Do you approve the change in the current state or not? I read "no" from your comment, but I'm not 100% sure @vivek Actually I've expected a response from you yesterday. Is there a chance you spin a new Stapler release with compatibility change revert in time? In such case it would be convenient. |
|
@jenkinsci/code-reviewers @reviewbybees Any other feedback? |
|
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. |
|
LGTM 🐝 👍 and all that sort of things. stapler imo is part of jenkins-core API (who else is using it?) and should be developed in sync. Getting 3 releases without being used by jenkins-core doesn't make sense to me. I wish stapler CI build would build latest tag from jenkins + stapler version patch to detect potential regressions. |
|
There was np technical reason in the release delay. The release process was I do not consider Stapler as a part of the Jenkins project, because the On Oct 30, 2016 13:28, "Nicolas De loof" [email protected] wrote:
|
|
🐝 and (as I am not "on the clock") 👍 |
|
FTR I have reviewed the changes to stapler and I do not view them as high or medium risk. I cannot see any use case for extending |
|
The way stapler is managed and hosted outside jenkins project doesn't make me feel it's not part of jenkins project (I agree it would make sense project is managed the same way). And I can't find any usage outside jenkins and KK's own projects :P |
|
@oleg-nenashev, let's say weak 👍 🐝 but I'll trust more knowledgeable persons here. |
|
Thanks for the feedback. |
oleg-nenashev
removed
needs-more-reviews
|
@vivek Please provide a human-readable changelog (maybe in Stapler's repo) |
|
@oleg-nenashev this is a team's decision. You don't have to take that burden alone if it happens to be necessary. Many people here have expressed their approval, so it's OK to merge IMO. Thanks for moving that forward and so many other things in that project Oleg. |
|
@vivek So what does this actually do? The changelog for this change in 2.28 is still useless. See @oleg-nenashev's question above. |
|
@daniel-beck @oleg-nenashev |
|
@vivek |
|
@oleg-nenashev sure, I can do that. I will send a PR with this change added to a CHANGELOG.md in stapler. |
Fixed regressions reported in #2415 and other enhancements.