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.

Sign up for GitHub

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

[JENKINS-55927] Hook event should not trigger Branch Indexing #908

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
+17 −11
Open

[JENKINS-55927] Hook event should not trigger Branch Indexing #908

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
0018451
[JENKINS-55927] Hook event should not trigger Branch Indexing
Dohbedoh Nov 8, 2024
5321736
[JENKINS-55927] Remove javadoc typo
Dohbedoh Nov 8, 2024
89eaa77
Merge branch 'master' into JENKINS-55927
Dohbedoh Nov 11, 2024
e745215
[JENKINS-55927] Disable the behavior by default. Add properties to re…
Dohbedoh Nov 13, 2024
d91fb40
Merge branch 'master' into JENKINS-55927
Dohbedoh Nov 13, 2024
101d3b5
Merge branch 'master' into JENKINS-55927
Dohbedoh Nov 15, 2024
2fb95c7
Merge branch 'master' into JENKINS-55927
Dohbedoh Nov 26, 2024
756bb42
Merge branch 'master' into JENKINS-55927
Dohbedoh Nov 27, 2024
5f842ac
Merge branch 'master' into JENKINS-55927
Dohbedoh Dec 2, 2024
78f8552
Merge branch 'master' into JENKINS-55927
Dohbedoh Dec 4, 2024
a35cfac
Merge branch 'master' into JENKINS-55927
Dohbedoh Dec 17, 2024
796da9a
Merge branch 'master' into JENKINS-55927
Dohbedoh Jan 6, 2025
ba8c034
[JENKINS-55927] Update deprecation description
Dohbedoh Jan 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/main/java/com/cloudbees/jenkins/plugins/bitbucket/hooks/HookProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,7 @@
* Abstract hook processor.
*
* Add new hook processors by extending this class and implement {@link #process(HookEventType, String, BitbucketType, String)},
* extract owner and repository name from the hook payload and then call {@link #scmSourceReIndex(String, String)}
* to launch a branch/PR reindexing on the matching SCMSource.
* extract details from the hook payload and then fire an {@link jenkins.scm.api.SCMEvent} to dispatch it to the SCM API.
*/
public abstract class HookProcessor {

Expand Down Expand Up @@ -86,10 +85,11 @@
/**
* To be called by implementations once the owner and the repository have been extracted from the payload.
*
* @deprecated SCM Event should not trigger Branch Indexing
Copy link
Member

@nfalco79 nfalco79 Jan 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

it's not possible, this will have a worster impact on issues like #539

Copy link
Contributor Author

@Dohbedoh Dohbedoh Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The impact really depend on a the environment. User that have large repositories with hundreds of branches / PRs cannot afford frequent branch indexing.
In case of #539, branch indexing is not a good solution IMO. A pull request decline event should not trigger branch indexing, but rather update the source and target based on discovery criteria through SCMHeadEvents.
Maybe the description could be improved.

Copy link
Member

@nfalco79 nfalco79 Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

but rather update the source and target based on discovery criteria through SCMHeadEvents.

This is hard because you should apply any registered trait (also external to this plugin) and simulate if any existing job is related to the target branch and in case delete or create.
In master I start to add unit test collecting some bitbucket cloud events and I'm providing test on how I expect they should do

* @param owner the repository owner as configured in the SCMSource
* @param repository the repository name as configured in the SCMSource
*/
protected void scmSourceReIndex(final String owner, final String repository) {

Check warning on line 92 in src/main/java/com/cloudbees/jenkins/plugins/bitbucket/hooks/HookProcessor.java

View check run for this annotation

ci.jenkins.io / Java Compiler

compiler:compile 

NORMAL:
deprecated item is not annotated with @deprecated
try (ACLContext context = ACL.as(ACL.SYSTEM)) {
boolean reindexed = false;
for (SCMSourceOwner scmOwner : SCMSourceOwners.all()) {
Expand Down
9 changes: 0 additions & 9 deletions ...ain/java/com/cloudbees/jenkins/plugins/bitbucket/hooks/NativeServerPushHookProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,15 +102,6 @@ public void process(HookEventType hookEvent, String payload, BitbucketType insta
return;
}

if (changes.isEmpty()) {
final String owner = repository.getOwnerName();
final String repositoryName = repository.getRepositoryName();
LOGGER.log(Level.INFO, "Received hook from Bitbucket. Processing push event on {0}/{1}",
new Object[] { owner, repositoryName });
scmSourceReIndex(owner, repositoryName);
return;
}

final Multimap<SCMEvent.Type, NativeServerChange> events = HashMultimap.create();
for (final NativeServerChange change : changes) {
final String type = change.getType();
Expand Down
9 changes: 0 additions & 9 deletions src/main/java/com/cloudbees/jenkins/plugins/bitbucket/hooks/PushHookProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@
import java.util.List;
import java.util.Map;
import java.util.concurrent.TimeUnit;
import java.util.logging.Level;
import java.util.logging.Logger;
import jenkins.plugins.git.AbstractGitSCMSource;
import jenkins.scm.api.SCMEvent;
Expand All @@ -71,13 +70,6 @@ public void process(HookEventType hookEvent, String payload, BitbucketType insta
push = BitbucketCloudWebhookPayload.pushEventFromPayload(payload);
}
if (push != null) {
String owner = push.getRepository().getOwnerName();
Copy link
Member

@nfalco79 nfalco79 Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

As described in JIRA issue I think these changes will cause to the user that have setup "Discover pull requests" strategies different than "The current pull request revision" they never get a build for PRs which target branch is the same branch subject of the merged.
If I understand correct the configuration in the JIRA issue the reported would trigger manual reindex to update all PRs. Than this should be archived using a build strategies to ignore some specific index events (maybe already implemented in other plugins)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The JIRA mentions the Received hook from Bitbucket. Processing push event on log line. per my reading, this is really coming from the event subscriber / hook processors (entry point is BitbucketSCMSourcePushHookReceiver). It would happen regardless of the multibranch configuration as far as I can tell.

final String repository = push.getRepository().getRepositoryName();
if (push.getChanges().isEmpty()) {
LOGGER.log(Level.INFO, "Received hook from Bitbucket. Processing push event on {0}/{1}",
new Object[]{owner, repository});
scmSourceReIndex(owner, repository);
} else {
SCMHeadEvent.Type type = null;
for (BitbucketPushEvent.Change change: push.getChanges()) {
if ((type == null || type == SCMEvent.Type.CREATED) && change.isCreated()) {
Expand Down Expand Up @@ -203,7 +195,6 @@ public boolean isMatch(@NonNull SCM scm) {
return false;
}
}, BitbucketSCMSource.getEventDelaySeconds(), TimeUnit.SECONDS);
}
}
}
}
Expand Down