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

processing the DELETE BRANCH message from GH after the GH Branch is deleted #245

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

Conversation

ghost
Copy link

@ghost ghost commented Nov 29, 2017

ok so the idea here is ... I'm in Github and I press "Delete Branch" and the message makes it to Jenkins BUT because the Github branch is already gone nothing interesting happens (rightly so the branch is gone ... it was just deleted) ---> BUT !!!! there's nothing saying I can't invoke a pipeline on a DIFFERENT BRANCH that contains clean up code.

Where I'm going with all this is I have a branch (call it INFRASTRUCTURE_CLEANUP) and that branch NEVER goes away (well it better not) as it contains pipeline code to cleanup infrastructure. When I delete ANY OTHER BRANCH of the same project I want my INFRASTRUCTURE_CLEANUP branch called.

As a side note ... I'm on the fence about "synchronizing" the doRun(String branch) method (cause synchronizing an entire method IS BAD !). Anyways I put a big comment in front of the method ... and I apologize if my coding is less than par. Trust me I'm trying my best so guidance is appreciated !


This change is Reviewable

@codecov-io
Copy link

codecov-io commented Nov 29, 2017

Codecov Report

Merging #245 into master will decrease coverage by 0.46%.
The diff coverage is 17.64%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #245      +/-   ##
============================================
- Coverage     56.24%   55.77%   -0.47%     
+ Complexity      639      635       -4     
============================================
  Files           117      117              
  Lines          3060     3071      +11     
  Branches        253      255       +2     
============================================
- Hits           1721     1713       -8     
- Misses         1244     1263      +19     
  Partials         95       95
Impacted Files Coverage Δ Complexity Δ
...hub/integration/branch/GitHubBranchRepository.java 7.69% <0%> (-0.21%) 6 <0> (ø)
...github/integration/branch/GitHubBranchTrigger.java 61.19% <21.42%> (-7.86%) 21 <0> (-1)
...tyasha/github/integration/branch/GitHubBranch.java 33.33% <0%> (-6.67%) 1% <0%> (-1%)
...integration/branch/webhook/GHBranchSubscriber.java 27.9% <0%> (-4.66%) 3% <0%> (-2%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba07740...1a29813. Read the comment docs.

@@ -1,15 +0,0 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Owner

Choose a reason for hiding this comment

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

please restore idea settings

@@ -10,7 +10,7 @@

<!-- historical name -->
<artifactId>github-pullrequest</artifactId>
<version>0.1.0-SNAPSHOT</version>
<version>0.1.2-SNAPSHOT</version>
Copy link
Owner

Choose a reason for hiding this comment

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

please remove version change

@@ -82,6 +82,12 @@ public String getUrlName() {
}


public synchronized void removeBranch(String branchName) {
if (nonNull(branches)) {
Copy link
Owner

Choose a reason for hiding this comment

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

you can just getBranches().remove(branchName) and i guess @Nonnull String branchName argument

@KostyaSha
Copy link
Owner

Found related issue https://issues.jenkins-ci.org/browse/JENKINS-40606

@KostyaSha
Copy link
Owner

i changed locally...

@KostyaSha
Copy link
Owner

#248
I think event arguments can't handle verification... they need know branchName...

ahhh I don't know how this got removed ... did my idea nuke it and then i committed ? I apologize.
@ghost
Copy link
Author

ghost commented Dec 1, 2017

Im stuck I don't understand the build failure ...

https://travis-ci.org/KostyaSha/github-integration-plugin/builds/309844133?utm_source=github_status&utm_medium=notification ->>>

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.3:compile (default-compile) on project github-pullrequest: Compilation failure
[ERROR] /home/travis/build/KostyaSha/github-integration-plugin/github-pullrequest-plugin/src/main/java/com/github/kostyasha/github/integration/branch/GitHubBranchTrigger.java:[236,60] cannot find symbol
[ERROR] symbol: variable branch
[ERROR] location: class com.github.kostyasha.github.integration.branch.GitHubBranchTrigger

but when I look at the code I don't see how it's undefined ... :( Line 236 is within a method that clearly identifies the variable "branch" on the method signature :

private List<GitHubBranchCause> readyToBuildCauses(GitHubBranchRepository localRepository,
                                                   LoggingTaskListenerWrapper listener,
                                                   @Nullable String branch) {

@ghost
Copy link
Author

ghost commented Dec 1, 2017

oops ... i synce with the fork ... and now I see it ...

@KostyaSha
Copy link
Owner

master has bit refactored code, it should help to handle delete case. Master contains skeleton of multibranch implementation. Unfortuantely multibranch (scm-api & branch-api) can't support delete events, has conflict on FS when branch name matches to pr-xxx name. New multibranch API is broken by design....

@andreldm
Copy link

Arriving late to the party... This plugin works well when branches are created (I'm using cron polling not webhook), but not detecting deleted branches is a hindrance for my use case, automated creation and destruction of staging server instances. For this reason, I need to manually stop and remove containers. As soon as you have a preliminary fix let me know to give it a try.
Thanks.

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

Successfully merging this pull request may close these issues.

4 participants