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 support for branch-specific badges #10

Closed
wants to merge 9 commits into from

Conversation

Projects
None yet
@pascalw
Copy link

pascalw commented Aug 31, 2014

This PR adds support for serving branch-specific badges. This is very useful if you have jobs building multiple branches.

Currently I've only added support for Git. I'm new to Jenkins plugin development but I didn't find a SCM agnostic way to determine the branch name of a build. Please correct me if I'm wrong.

I've also introduced a AbstractBadgeAction class to share common functionality between the BadgeAction and PublicBadgeAction classes.

Please let me know if this is something you're interested in merging. I would love to get your feedback.

@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

cloudbees-pull-request-builder commented Aug 31, 2014

plugins » embeddable-build-status-plugin #20 SUCCESS
This pull request looks good

@jenkinsadmin

This comment has been minimized.

Copy link
Member

jenkinsadmin commented Sep 1, 2014

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@pickypg

This comment has been minimized.

Copy link

pickypg commented Dec 13, 2014

I'd love to see this get into the next release and use it on my project. ++

@pascalw

This comment has been minimized.

Copy link
Author

pascalw commented Dec 13, 2014

@pickypg I would also still like to see this merged, but because it has been ignored for so long it needs some rework now after #11 was merged.

I would first like to get a confirmation from one of the maintainers though that they would accept this PR before I go and spend the time to make this mergable again.

@mgedmin @christiangalsterer could one of you guys chime in on this?

@mgedmin

This comment has been minimized.

Copy link
Member

mgedmin commented Dec 15, 2014

I'm only a maintainer because I submitted a PR, got no response, then asked for the commit bit on the mailing list, as suggested by http://jenkins-ci.org/pull-request-greeting. I suggest you do the same.

I can try to review your diff, but I'm not really familiar with Jenkins internals or Java.

// this value can contain references to the remote etc.
String runBranchName = branch.getName().replaceFirst(".*/", "");

if(runBranchName.equals(branchName)) {

This comment has been minimized.

Copy link
@mgedmin

mgedmin Dec 15, 2014

Member

Some people name their branches with slashes inside, e.g. "mgedmin/fix-bug-1234". Stripping everything up to the last "/" would make it impossible to distinguish between them.

Perhaps check if branch.getName().equals(branchName) or branch.getName().endsWith("/" + branchName)? (This would make it impossible to distinguish "master" from "bob/master", but meh.)

This comment has been minimized.

Copy link
@pascalw

pascalw Dec 15, 2014

Author

Good point, I didn't think of that. I'll see if the Git plugin API provides a way to get the branch name without remote.


public class GitScmSupport {
public static BallColor getStatusForBranch(AbstractProject<?, ?> project, String branchName) {
if (Jenkins.getInstance().getPlugin("git") == null) {

This comment has been minimized.

Copy link
@mgedmin

mgedmin Dec 15, 2014

Member

So I don't really know Java. How does it deal with import hudson.plugins.git.Branch at the top of this file if the git plugin is missing?

This comment has been minimized.

Copy link
@pascalw

pascalw Dec 15, 2014

Author

I don't know exactly how Jenkins handles this, but the dependency on the Git plugin is marked optional. AFAIK import statements are only resolved at compile time, while at runtime classes are only loaded once you access them. Because the code that accesses Git plugin classes is guarded with that Jenkins.getInstance().getPlugin("git") == null statement there shouldn't be a problem.

See https://wiki.jenkins-ci.org/display/JENKINS/Dependencies+among+plugins for more information.

@pascalw

This comment has been minimized.

Copy link
Author

pascalw commented Dec 15, 2014

I'm only a maintainer because I submitted a PR, got no response, then asked for the commit bit on the mailing list, as suggested by http://jenkins-ci.org/pull-request-greeting. I suggest you do the same.

I can try to review your diff, but I'm not really familiar with Jenkins internals or Java.

Yes I saw that, but I was still hoping for some collaboration :-)

I think I'll indeed go ahead and request merge access, cause I think this is useful. It's a pity that there doesn't appear to be a way to make this branch checking SCM agnostic (at least that I could find), but we can also change that part to something more generic should we find something like that exists.

@christiangalsterer

This comment has been minimized.

Copy link
Contributor

christiangalsterer commented Dec 15, 2014

Similar as @pascalw I also became maintainer as my PR was not handled in time and I applied for merge permissions. So maybe you same the same way and also apply for merge permissions. Nonetheless will have an look on your PR and happy to assist.

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Dec 15, 2014

I think this should be rejected. If you need a distinct status for each branch, you should have a distinct job for each branch, so that lastCompletedBuild (etc.) are meaningful without special tricks or hardcoded dependencies on particular SCMs from this plugin. There are various ways of creating multiple jobs for branches, such as the Job DSL plugin, but the cleanest approach is to use a project type integrated with the Multi-Branch API plugin.

@pascalw

This comment has been minimized.

Copy link
Author

pascalw commented Dec 15, 2014

@jglick thanks for your feedback. I'm not entirely sure if I agree with you though. In a way I can understand what your saying but on the other hand I think it's pretty common to have a single job that builds both develop and master branches for example.

IMO this PR is a view on the build status of a job. It already features a filter to filter a specific build and this would just be another filter. Could you agree with that standpoint looking at it from this angle?

I agree that it's a pity that there's a dependency on the Git plugin and thus only Git will be supported currently. Maybe this could be worked around with an extension point so SCM plugins can opt-in to this plugin, but I'm not sure if that would make sense.

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Dec 15, 2014

I think it's pretty common to have a single job that builds both develop and master branches

This is what I am arguing should be rejected as a valid use case on architectural grounds, lest every plugin out there which deals with build histories have to be made more complex and have otherwise unnecessary dependencies on SCM plugin implementations. In other words, the user requirement should be solved differently, via per-branch projects. (CC @stephenc)

@pascalw

This comment has been minimized.

Copy link
Author

pascalw commented Dec 16, 2014

I was not aware that configuring jobs with multiple branches goes against Jenkins architecture. I can totally see your point though and there's certainly some elegance in having a job per branch.

I do think that at the moment such a setup is not very well supported without significantly changing how jobs are configured. For the short/medium term I will just stick with my fork of this plugin to accommodate my workflow but for the long term I'll definitely look into having a job per branch.

@christiangalsterer

This comment has been minimized.

Copy link
Contributor

christiangalsterer commented Dec 16, 2014

My two cents: I tend to agree with @jglick that this PR should be considered to be rejected. Although building two branches with the same job is possible, it has some pitfalls (unclear build status, build trend and others) and therefore would rather suggest to split the job into a single job for each branch.

@suryagaddipati

This comment has been minimized.

Copy link
Member

suryagaddipati commented Dec 16, 2014

We have a single job for all branches using https://github.com/groupon/DotCi .
We use this plugin for build status and DotCi allows users to pass in branch via branch param to the url via ?branch=<branch-name> . It also neatly defaults to default branch of github repo if no branch is passed.

Also, Build Status defaults to default branch and history is linear per branch.

I think its impractical to setup a build per branch, esp if you are using git which encourages users to create ephemeral branches. We have around 400 branches in one of our repos, it would be impractical to setup 400 jenkins jobs for the project.

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Dec 16, 2014

it would be impractical to setup 400 jenkins jobs for the project

Of course it would be. That is why a project using the Branch API plugin generates a new subproject automatically when a new branch is detected in the repository. (And deletes the subproject according to a configurable policy when the branch is closed.)

To be clear, I am not the maintainer of this plugin and not in charge of making decisions about whether the value of a new feature justifies its “weight” in the code. This PR just happened to catch my eye and I wanted to make a general comment about how I think this sort of request should be satisfied going forward.

@tfitch

This comment has been minimized.

Copy link

tfitch commented Dec 19, 2014

I agree with @jglick here. I never create a job that builds all branches. The easiest way to describe it as a problem is build 9 is for a feature branch and broken. And then build 10 is for the master branch and works. Is the build fixed? No. Does it look fixed to any non-developer that looks at the project's build status? Yes. And that is why it fails.

It might work for small projects, but as the code or team grows using a single job doesn't scale. And once you have multiple jobs this PR then becomes a moot point. In a sense, this PR would encourage bad job configurations. That's my two cents.

@pascalw

This comment has been minimized.

Copy link
Author

pascalw commented Dec 19, 2014

I guess it really depends on how you've set things up. It works well for my use cases but I understand this may not be desirable for larger scale setups.

@pascalw pascalw closed this Dec 19, 2014

@kohsuke

This comment has been minimized.

Copy link
Member

kohsuke commented May 1, 2015

This also impacts pull request builder. See jenkins-infra/backend-jenkins-plugin-info-plugin#3 that led me here.

I understand where Jesse is coming from, but branch-api doesn't even have a released version available, so the solution we are talking about isn't applicable for the majority of users. Rejecting something because we think we want to do something better seems unfair for people who are trying to scratch the itch.

I think the trick is to make it pluggable. I do have a problem of this plugin special casing git plugin (which the submitter acknowledges), but if it's generalized so that the filtering logic is made pluggable, then the git specific logic can be moved somewhere else and we are all happy.

@stephenc

This comment has been minimized.

Copy link
Member

stephenc commented May 1, 2015

That just says to me that we need to look into getting a release of branch-api out. IIRC the blocker from my PoV was being able to give everyone an ability to wrap jobs with an untrusted build wrapper.

Now that there's docker and spoon container technology more widely available I believe it becomes possible to give people on linux and windows container wrapped builds, with chroot jails being a reasonable fall-back for non-linux based builds.

So I think perhaps we (or more correctly I) could spin a few cycles getting it up to a full 1.0

@pascalw

This comment has been minimized.

Copy link
Author

pascalw commented May 1, 2015

@kohsuke thanks for your input on this matter. After being introduced to the multi branch api concept by Jesse I've played around with the multi-branch-project-plugin which, as I understand is a third party implementation of the concept and not even using the actual multi branch API, but I've come to really like the concept. I think it would be absolutely great if we could get a stable version of the branch api as @stephenc mentions in the near future so we can make a proper implementation of multi branch projects.

I'm not sure what timeframe we're talking about though so it may still make sense to make the branch filtering pluggable and ship it like this for the time being. Please do let me know how you want to move forward.

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Aug 27, 2015

BTW there is ongoing work on this, in case you missed it. Blog I am still not quite ready for a non-beta release but I think it is getting closer.

@pascalw

This comment has been minimized.

Copy link
Author

pascalw commented Aug 28, 2015

Nice work @jglick! Looking forward to play with it a bit.

@jdamick

This comment has been minimized.

Copy link
Member

jdamick commented Apr 12, 2016

where does this stand, it's been a long time with no suitable resolution? It is currently very confusing if you have PRs that are built by a single job but the badge says "failure" but really it means a PR failed. I agree with @kohsuke whether this is "correct" or not shouldnt negate the fact that this is a very common practice and a needed feature..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.