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

Send build completion notification for pull requests merged after build start #472

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

mguillem
Copy link

This PR fixes the problem of notifications beeing not sent to Bitbucket at the end of a build causing BitBucket to show for ever "1 Build in progress".

Steps to reproduce the problem:

  • create pull request in BitBucket with an associated "long build"
    • build starts in Jenkins
    • Jenkins sends the status INPROGRESS
    • Bitbucket shows "1 Build in progress"
  • merge PR in BitBucket
    • Jenkins receives a notification and the SCMSource associated with the running build becomes jenkins.scm.impl.NullSCMSource
  • build ends in Jenkins but no notification is send due SCMSource of the build now being NullSCMSource
  • BitBucket shows for ever "1 Build in progress" for the pull request and the associated commit

Your checklist for this pull request

  • Make sure you are requesting to pull a topic/feature/bugfix branch (right side) and not your master branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or in Jenkins JIRA
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Did you provide a test-case? That demonstrates feature works or fixes the issue.

The provided fix searches for the SCMSource in the enclosing job if the build now contains a NullSCMSource.

// for instance PR merged on Bitbucket since the build has been started
ItemGroup<?> grandFather = build.getParent().getParent();
if (grandFather instanceof SCMSourceOwner) {
s = ((SCMSourceOwner) grandFather).getSCMSources().get(0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a Jenkins multibranch job has multiple SCM sources (e.g. an original repository and a fork with distinct branch names), I think this might not select the correct one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, because this plugin is using the older REST API that does not distinguish between repositories, it will work anyway, as long as the repositories are on the same server.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But a similar fix might not be valid for jenkinsci/github-checks-plugin#196 because the GitHub API Create a check run is at POST /repos/{owner}/{repo}/check-runs and presumably has to be told the correct repository.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix worked for me and was quite easy to implement. A solution to be really safe would be probably to save the url to which the INPROGRESS event has been sent to reuse it at build completion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not only the URL but also the credential ID.

I think this change is fine for bitbucket-branch-source-plugin, but we need something more reliable for the equivalent issue in github-checks-plugin and in the future bitbucket-server-checks-plugin.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can someone say whether the proposed fix can be merged (and do it) or otherwise provide feedback about what should be changed? Thanks.

@mguillem
Copy link
Author

mguillem commented Sep 9, 2021

Hi maintainers, sorry to ask again but could one of you merge this PR or tell what should be changed? Thanks.

@KalleOlaviNiemitalo
Copy link
Contributor

JENKINS-66620 was filed today for the problem that this PR is intended to fix.

@mguillem
Copy link
Author

@car-roll, @bitwiseman could you have a look at this PR and merge it or provide feedback if it should be improved/changed?Thanks.

@lifeofguenter
Copy link
Contributor

I don't know currently enough around this to do a comfortable merge - maybe someone else could chip-in?

What would help would to have some tests though 😅

@mguillem
Copy link
Author

What would help would to have some tests though 😅

@lifeofguenter can you point me on some existing tests that could help to get started here?

@KalleOlaviNiemitalo
Copy link
Contributor

KalleOlaviNiemitalo commented Nov 17, 2021

This will not work reliably for multibranch projects that have multiple branch sources, if the plugin is ever changed to use Bitbucket Server's newer build-status REST API that needs the project and repository slugs in the URL. The newer API is better in that build statuses posted to it can be deleted later (BSERV-11393, #448 (comment)). However, even the newer API would work more reliably with this PR than without this PR.

@mguillem
Copy link
Author

@lifeofguenter I really appreciate your activity for this plugin since a few days but it's quite depressing if you say that you don't feel qualified to review this PR. It is waiting since > 4 months and until now no maintainer has had time to have a look at it.

Is there any change I can make to make you feel more comfortable with my fix?

@lifeofguenter
Copy link
Contributor

@mguillem I think you can be fair enough to give me time to review it properly or add tests for the behavior prior and expected behavior afterwards if you want an instant merge. Wdyt?

@mguillem
Copy link
Author

@lifeofguenter I'm more than happy if you plan to look at this PR soon. Your previous comment has wrongly given me a false impression. Sorry for that.

@mischoem
Copy link

mischoem commented May 24, 2022

Hi @mguillem @lifeofguenter,

we (my company and I) would have nice benefits, if this feature will be released. May you give us a short prospect if and when work will continue?

Kind regards Michael

feature/push-notification-for-merged-pull-request

# Conflicts:
#	src/main/java/com/cloudbees/jenkins/plugins/bitbucket/BitbucketBuildStatusNotifications.java
@mguillem
Copy link
Author

@bitwiseman, @lifeofguenter, @car-roll could you perhaps provide some feedback here? I'm ready to ensure that the PR becomes "mergeable" again but it is only worth the effort if somebody looks at it. Thanks in advance.

@twingg
Copy link

twingg commented Nov 3, 2023

I think a lot of users are waiting for this PR to be merged)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants