feature/324 | skip non PR notifications #451

sams-gleb · 2021-05-25T12:30:31Z

Adding a checkbox to avoid overwriting of PR status by the branch build status. Should fix #324 . We had similar problems in our pipelines where branch build can be finished in the middle of running PR build thus allowing to merge un-tested changes into master

Your checklist for this pull request

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

KalleOlaviNiemitalo · 2021-11-25T22:28:59Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/BitbucketBuildStatusNotifications.java

+                BitbucketBuildStatuses buildStatuses = bitbucket.getBuildStatus(hash);
+                for (BitbucketBuildStatus bs : buildStatuses.getValues()) {


Looks like this will blow up if getBuildStatus returns null.

KalleOlaviNiemitalo · 2021-11-25T22:30:54Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/BitbucketBuildStatusNotifications.java

@@ -157,7 +161,7 @@ private static void createStatus(@NonNull Run<?, ?> build, @NonNull TaskListener
            statusDescription = StringUtils.defaultIfBlank(buildDescription, "The build is in progress...");
            state = INPROGRESS_STATE;
        }
-        status = new BitbucketBuildStatus(hash, statusDescription, state, url, key, name);
+        status = new BitbucketBuildStatus(hash, statusDescription, state, url, DigestUtils.md5Hex(key), name);


The added md5Hex call looked surprising but I now see you just moved it from the BitbucketBuildStatus constructor.

looks like it was added to avoid issues with longer names #15

KalleOlaviNiemitalo · 2021-11-25T22:35:37Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/client/BitbucketCloudApiClient.java

+    @Override
+    public BitbucketBuildStatuses getBuildStatus(@NonNull String hash) throws IOException, InterruptedException {
+        return null;
+    }
+


And here it does return null, causing an exception in the caller. This should preferably be implemented, or at least the caller should check for null.

KalleOlaviNiemitalo · 2021-11-25T22:50:02Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/BitbucketBuildStatusNotifications.java

@@ -199,6 +204,15 @@ private static void sendNotifications(BitbucketSCMSource source, Run<?, ?> build
            listener.getLogger().println("[Bitbucket] Notifying commit build result");
            key = getBuildKey(build, r.getHead().getName(), shareBuildKeyBetweenBranchAndPR);
            bitbucket = source.buildBitbucketClient();
+            if (sourceContext.doNotOverridePrBuildStatuses()) {


Should this have a special case for reporting the status of a pull request build?

Like, there is a pull request and it's first built once and the build status goes to Bitbucket all right. Then a user of Jenkins replays the build — there is no new commit. When the second build finishes, does the plugin decide not to report the status to Bitbucket because that commit already has one build status from a pull request — even though it's from the same pull request? It seems to me that the previous build status should be overwritten in that case.

for this case it shouldn't be a problem as notification for PRs being processed in the previous if statement, this is for "branches" only, you could check line 198

KalleOlaviNiemitalo · 2021-11-25T22:53:19Z

.../com/cloudbees/jenkins/plugins/bitbucket/BitbucketBuildStatusNotificationsTrait/config.jelly

+   <f:entry title="${%Do not override PR build statuses}" field="doNotOverridePrBuildStatuses">
+    <f:checkbox/>
+  </f:entry>


I would like a longer help text that describes what this setting does and why one would want to use it. Yes the information is in #324, but it's nicer to have it in Jenkins as well.

I meant, in src/main/resources/com/cloudbees/jenkins/plugins/bitbucket/BitbucketBuildStatusNotificationsTrait/help-doNotOverridePrBuildStatuses.html.

thanks, added

KalleOlaviNiemitalo · 2021-11-25T22:59:07Z

Overall though, I feel the better fix would be to (1) abort the branch build as soon as Jenkins finds that there is a PR from that branch as well and (2) delete the INPROGRESS build status from Bitbucket when the build is aborted that way.

sams-gleb · 2021-11-29T09:30:51Z

Overall though, I feel the better fix would be to (1) abort the branch build as soon as Jenkins finds that there is a PR from that branch as well and (2) delete the INPROGRESS build status from Bitbucket when the build is aborted that way.

Thanks for the review! Makes sense, I will look when I have time and mb will close this PR

sams-gleb · 2021-11-29T15:05:55Z

Overall though, I feel the better fix would be to (1) abort the branch build as soon as Jenkins finds that there is a PR from that branch as well and (2) delete the INPROGRESS build status from Bitbucket when the build is aborted that way.

I took another quick look.. The main use case here is when we create a PR and still having a running branch pipeline which could overwrite PR status thus non-tested changes could be merged. Branches and PRs do share key so it might be not so simple to delete only in progress branch status, in many cases it might be already in progress status of PR

alexanderrtaylor · 2023-01-27T13:10:23Z

Those failing builds are not legit. It appears that the filename has gotten too long due to the length of the git commit hash when checking out inside of windows.

That being said, I am not sure this feature is all that useful @sams-gleb because what about things like main/versioned branches that you do want to know build status on?

To me instead of a checkbox that ignores anything but PR build status, I think it should be a regex which would let you ignore status notifications from anything not-desired. To me that is a more useful feature and also you will most likely have to add a test before this gets approved and merged in. Because this is something super useful to me I am going to try and help make some code changes to see if I can get it working

sams-gleb force-pushed the feature/324-pr-notifications branch 2 times, most recently from 5b04669 to 66655e0 Compare November 23, 2021 09:48

sams-gleb marked this pull request as ready for review November 23, 2021 09:52

sams-gleb force-pushed the feature/324-pr-notifications branch 2 times, most recently from 90743cc to 2447461 Compare November 23, 2021 11:33

KalleOlaviNiemitalo suggested changes Nov 25, 2021

View reviewed changes

sams-gleb force-pushed the feature/324-pr-notifications branch from 2447461 to 087a59c Compare November 29, 2021 14:46

sams-gleb force-pushed the feature/324-pr-notifications branch 2 times, most recently from dbbce48 to d2999e0 Compare November 30, 2021 06:00

feature/324 | Skip branch notifications once PR build has started

d3a1c73

sams-gleb force-pushed the feature/324-pr-notifications branch from d2999e0 to d3a1c73 Compare November 30, 2021 09:29

sams-gleb and others added 2 commits November 30, 2021 15:34

adding help file

9137f15

Merge branch 'master' into feature/324-pr-notifications

f1c46b0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature/324 | skip non PR notifications #451

feature/324 | skip non PR notifications #451

sams-gleb commented May 25, 2021 •

edited

KalleOlaviNiemitalo Nov 25, 2021

KalleOlaviNiemitalo Nov 25, 2021

sams-gleb Nov 29, 2021

KalleOlaviNiemitalo Nov 25, 2021

KalleOlaviNiemitalo Nov 25, 2021

sams-gleb Nov 29, 2021

KalleOlaviNiemitalo Nov 25, 2021

KalleOlaviNiemitalo Nov 30, 2021

sams-gleb Dec 1, 2021

KalleOlaviNiemitalo commented Nov 25, 2021

sams-gleb commented Nov 29, 2021

sams-gleb commented Nov 29, 2021

alexanderrtaylor commented Jan 27, 2023

		BitbucketBuildStatuses buildStatuses = bitbucket.getBuildStatus(hash);
		for (BitbucketBuildStatus bs : buildStatuses.getValues()) {

feature/324 | skip non PR notifications #451

Are you sure you want to change the base?

feature/324 | skip non PR notifications #451

Conversation

sams-gleb commented May 25, 2021 • edited

Your checklist for this pull request

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KalleOlaviNiemitalo commented Nov 25, 2021

sams-gleb commented Nov 29, 2021

sams-gleb commented Nov 29, 2021

alexanderrtaylor commented Jan 27, 2023

sams-gleb commented May 25, 2021 •

edited