-
-
Notifications
You must be signed in to change notification settings - Fork 55
Pipeline support #69
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
Pipeline support #69
Conversation
|
Thanks for taking this up (I am the original author of ea4a885). If there's any questions I can help answer feel free to ping me here or email me directly. |
Thank you, I've updated the description. Sorry for misattributing the original work! I went through the code line-by-line, it's actually a very clean implementation. I made it even less intrusive by requiring Job to implement I also made changes to reduce interdependencies between classes. That way, I could split the commit into smaller parts. The customized messages cannot even be configured in the pipeline, so I don't consider it a defect in this PR. Rather, we should find a way to post arbitrary messages to Stash from the pipeline, which would be a large separate task. |
|
I have fixed a major bug I introduced while applying to original patch manually (enabled projects were treated as disabled and vice versa). I've done some testing, everything appears to be working correctly. Now it's time for others to test. |
|
Changes to variable names and comments only. |
|
@proski Thank you - I appreciate the kind words. :) One of the ideas is to revamp the way that this plug-in tracks build status - it currently traverses the comment chain to determine when things need to build and this ends up not being very efficient at scale - this could be switched to use the native build status API in Bitbucket Server. That would reduce the size of this codebase, make it much faster on more active repos, and also take advantage of build status API which currently requires adding the stash notifier plugin if you want to use it. Another is adding native support for Bitbucket Server webhooks - this would eliminate polling completely and also instantly start builds when pull requests triggered them instead of having to heavily poll the server via the comments API. If you think this would be something you be open to let me know, and I can dig in as well. :) |
|
(also also, if you like, Git maintains author and committer separately in commits, though they are normally the same - this is the use case it helps track. When you are rebasing you can do a |
|
@rhencke, I've set your authorship on the commits that are derived from your patch. It's good to know that your patch is heavily used. My changes are mostly to avoid any fallbacks. For instance, I was uncomfortable about passing PRs are getting approved slowly here, so I don't feel good about asking you to do more changes that may wait for a long time. I believe traversing the comments is unavoidable with the current design, as the plugin is looking for "test this please" and "NO test". We can tell the plugin to stop looking for PRs with the successful build status, but then "test this please" would stop working for the projects that have been tested successfully. It's not a typical case ("test this please" is more commonly used in case of build failure), but it can be confusing to the users. Speaking of the build status, do you set it from the pipeline? Stash Notifier is a post-build publisher, it cannot be configured from the pipeline GUI. Are you doing it from the pipeline code? If we add Stash Notifier functionality to this plugin, we could configure it from the GUI. I believe Bitbucket plugin supports webhooks, so it may be better just to migrate to it. But the last commit in the code was on December 6, 2017. https://github.com/jenkinsci/bitbucket-plugin There is also Bitbucket pull request builder. It is being actively developed. It only claims to support Bitbucket.org in README, but I see a reference to Bitbucket server in the code. |
|
Hey @proski :)
That's fair - that sounds like a good idea. I'll be the first to admit the Jenkins API is not my strong suit, so please feel free to improve as you see fit. :)
That's okay! The fact they're getting approved and merged at all is enough for me. :)
I agree, this is true for polling style, unfortunately. But, if web hook support was implemented (would be happy to work on this), then for those who enabled it, Bitbucket Server will just POST the comments to Jenkins as they happened, and all the build would have to do is look at each comment as it arrived. (basically, it'd be a model akin to /git/notifyCommit from the Git plugin, but specific to receiving events for Bitbucket).
Yes - exactly. I don't have the code in front of me. It's not pretty, but it's something to the effect of (pseudocode): StashNotifier.notify('INPROGRESS')
def error
try {
/* build */
} catch(err) {
error = err
throw err
} finally {
if (error) {
try {
StashNotifier.notify('FAILED', error)
catch (err) {
error.addSuppressed(err)
}
} else {
StashNotifier.notify('SUCCESS')
}
}...which I would love to delete and just configure from the pipeline GUI instead. :)
Unfortunately, yes. I can't speak for others, but at least in my environment we end up having to install several plugins together (this one, Stash notifier, etc) to stitch together a solution. But it's honestly been a while since I went and looked at what else was available, and tried the latest versions of everything - maybe there's better options now for where some of this work could be focused, or maybe there's something out there that'd be a good swap-out replacement.
I'll check that out - thank you! :) |
|
@rhencke, thank you for your detailed reply. There are some recipes in the StashNotifier README Scripting is a mixed bag. It's a barrier for entry, but it gives a lot of power if it's needed. Scripting works with multiple components just fine, but GUI doesn't work well if there are multiple components with their own configurations. Either there should be one plugin dealing with Stash-Jenkins interactions, or there should be scriptable low-level plugins and a high-level GUI capable plugin that depends on them. I also plan to test every plugin with "Bitbucket" in its name as long as it's maintained or at least has an active community. Back to this PR, my only unresolved concern so far is whether the environment variables are provided to all jobs in the pipeline. I want to have a way to identify the top level project for every Run. That's not your code, the environment variables were handled as parameters until recently. |
|
@MyroslavSeliverstov, I'm sorry, I don't understand either issue. Even the GUI in the screenshots looks unfamiliar to me, except the Stash comments. I don't think those issues can be fixed in this PR. This PR allows Stash Pull Request Builder to operate on pipelines by using less specific interfaces. There are no other changes here. In particulate, there are no changes to how comments are posted. There is one change that could actually fix the disappearing job. That's the change from Let's get back to those issues once this PR gets merged. Jenkins logs should help figure out what is happening. |
|
I've added commits from PRs #67, #72 and #73 to provide full pipeline support. This PR is marked WIP anyway, we can always merge other PRs and rebase this one to exclude already merged changes. I have found a document how to write a pipeline-compatible plugin: https://jenkins.io/doc/developer/plugin-development/pipeline-integration/ I plan to go through it carefully, it looks like some issues still need to be addressed, e.g. changing |
I've removed the commits that come from other PRs. For the purpose of testing, I have a branch in my fork that integrates all those PRs and some other work (mostly warning fixes): https://github.com/proski/stash-pullrequest-builder-plugin/tree/merge-all I'm open to splitting this PR even further (e.g. taking my environment commit elsewhere) if it helps with merging. |
I've spent some time trying to get https://wiki.jenkins.io/display/JENKINS/Bitbucket+Branch+Source+Plugin to work. I'm still seeing some issues, probably because we are stuck using a 4.x version of Stash/BB You can configure it with a dsl-like the one seen below. The code is not pretty with all the configuration blocks, but more importantly:
I appreciate all the work done here, and I'm sad to be making a fuss here. It seems to me that the branch source/organization approach is "the way to go" with Pipelines. This is only a gut feeling though. I would be happy to see arguments that show otherwise, or use cases that are not supposed to be/can't be handled by the branch source approach. Even if this plugin is not going to support pipelines I think it's still going to be useful for many years to come. I don't expect the old-style jobs to disappear any time soon. |
|
In previous version, a Pull Request triggered with the build phrase would rebuild if there was a commit to the branch trying to be merged to master. In this pull request version, the branch has new commits but isn't building unless the build phrase is entered again |
|
@d8v1dee, @MyroslavSeliverstov, I see several issues with Jenkins 2.172. Exception on startup for every project using Stash PR trigger, pipeline or not: Probably related, logged periodically for every project with Stash PR Trigger: Only for pipelines, a crash when removing the old comment: My priority is to fix support for non-pipeline projects. I think I have an idea how to fix the pipeline specific issue, but it has to wait. I pushed a fix, then I realized that there is a bigger issue, so I removed that fix for now. |
|
@jakub-bochenski, I appreciate your post about Bitbucket Branch Source plugin. I still need to try it with my own hands before I can discuss it in details. However, I see that supporting pipelines in this plugin can provide a stepping stone from old-style projects to the pipelines. I see a lot of interest, I see many testers here. And the code additions are minimal, most changes are replacing one class with another. I'll try to extract all the good stuff from this PR in case we don't merge it (such as But first I need to fix the recent breakage. Either Jenkins released a broken version, or we should stop using the obsolete |
I'm reluctant to include duplicate functionality here, however if there is much call for it I won't block it. |
|
My fix for The master branch is OK. Even with this branch as of now, all jobs except the pipelines are working. I'm looking for a fix for |
|
I have added the fix. Here's what was happening.
The fix makes Likewise, While I don't consider the fix particularly elegant, I believe that Jenkins is behaving unreasonably, and the entry points for the trigger should protect the internal state of the trigger. Eventually we could restructure the code to be closer to the Jenkins concepts (e.g. an object associated with the job, an object associated with the run), then we could take a less defensive stance. |
|
Pushed a much simpler and nicer fix for the failure to replace the BuildStarted comment. That's what I attempted initially, but one little piece was missing. That piece has also been submitted as #79. |
src/main/java/stashpullrequestbuilder/stashpullrequestbuilder/StashBuildTrigger.java
Outdated
Show resolved
Hide resolved
|
Looks like with this PR pipeline defined parameters will overwrite plugin supplied variables. |
|
@proski when we can expect this PR to be merged? |
I have no idea. I think we are getting close. Many parts of the original PR are already in the master branch, including one little piece that was merged today. The remaining parts are getting more straightforward an easy to review. I'm going to remove the commit that deals with the environment variables, as it's not strictly necessary for the minimal pipeline support. It will be resubmitted separately. On the other hand, we have a security issue (#85) and a fix for a server-killing bug (#93) in review, so changes are they might be merged before this PR. |
|
Added unit tests. |
|
Rebased on top of the master branch. Added an extra commit that prevents unit tests from breaking. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I'm listed as author, but I think you get the credit for commit
58416e6 :) Much as it would have been a good idea for me to add better testing...)
Thanks. I used my script |
|
Significant improvement in the I hope this PR is applied soon. We can always perfection the code in separate PRs. But before this PR is applied, all further work on the pipeline support is essentially blocked. We are turning away potential contributors, because their PRs are not going anywhere. |
|
Fixed an issue with the global enable checkbox not being populated from the settings (it was always showing as unchecked after loading the system configuration page). It turns out the getter |
|
Improvements in the last commit (global option to enable plugin support):
|
|
Thank you for taking the time to implement pipeline support. I recently updated this plugin on our company Jenkins installation and noticed that pipeline support was completely disabled. We've reverted to the older version and are since waiting for the merge and release of this PR. If there is anything I can do to help speed up the process, please let me know. |
@HB-2012: thank you for reporting the issue! The pipeline support was disabled because the code is not ready to support it. It's better not to show the trigger in GUI if it won't work. Why did you need to revert to an older version? Did it resolve any issue for you? What were the versions? |
|
How far is this PR from being merged? Any possibility to be included in the next version? |
|
1 of the commits have already been merged, 2 others have been approved by me and are waiting for review by @jimklimov |
The reason we had to revert is because we were on an older version that didn't have the check. Some teams are using this plugin with additional (Power)shell scripts to make it all function somewhat decently. We run quite a lot of plugins and workflows so I'm unsure of the specific use cases, I just know that our users wanted to downgrade after this plugin no longer worked inside a pipeline. Looking forward to the release! :) |
@HB-2012: Thank you for the reply. It's actually good that pipeline support was the only reason to revert to an older version. It means that merging this PR would likely fix your setup. I don't understand how it's possible to use a plugin without pipeline support with pipelines. The only explanation I have is that the Jenkins Pipeline code has a compatibility mode for plugins that only support freestyle projects. Perhaps that mode was activated with the scripts you mentioned. |
|
This PR is down to one commit now. Getting really close! |
|
Added a commit to rename the descriptor class, also submitted separately as #116. |
Add a global option to enable pipeline support. If it's enabled, don't require the job to inherit from AbstractProject. Allow any Job subclass that implements ParameterizedJob, which is true both for freestyle projects and for pipelines. Make it clear in the option help text and in README.md that pipeline support is currently experimental. Add workflow-job as a test dependency. Add unit tests to make sure that WorkflowJob is supported only when pipeline support is enabled.




This comment is here to reflect the current state of the PR, it is updated after every push
This is a rebased commit by @rhencke to support pipelines. It has been successfully tested both with pipelines and with traditional projects (Free Style, Matrix and Maven).
Most changes are already in the master branch. The remaining part is a commit to enable pipeline support in global configuration. That is necessary to make users understand that the support is incomplete and there are other ways to access Bitbucket Server from a pipeline. Also, a section about pipeline has been added to
README.md.