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

[JENKINS-29066] potential fix #331

Merged
merged 4 commits into from Jul 6, 2015

Conversation

Projects
None yet
6 participants
@ndeloof
Copy link
Member

ndeloof commented Jun 25, 2015

I was able to reproduce JENKINS-29066 using github.com/ndeloof/oki-docki-demo as a repository. JGit does return /ref/pull/1/merge as a candidate revision during polling, which is ignored by BuildChooser when looking for revisions to build, resulting in an infinite build loop.
This commit do ensure polling do only consider the configured refspecs. Probably need to also review the BuildChooser extension point to ensure it uses the same logic to avoid such issues

for (BranchSpec branchSpec : getBranches()) {
HEADS:

This comment has been minimized.

Copy link
@KostyaSha

KostyaSha Jun 25, 2015

Member

Where it used?

This comment has been minimized.

Copy link
@ndeloof

ndeloof Jun 25, 2015

Author Member

useless indeed

@jenkinsadmin

This comment has been minimized.

Copy link
Member

jenkinsadmin commented Jun 25, 2015

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

@ndeloof

This comment has been minimized.

Copy link
Member Author

ndeloof commented Jun 26, 2015

@MarkEWaite

This comment has been minimized.

Copy link

MarkEWaite commented Jun 29, 2015

I think it is ready to merge, though the first commit seems to be solving a different problem. Wouldn't it be better to rephrase the commit message on the first commit to remove the reference to JENKINS-29066?

Would you also consider an additional test case that I wrote while trying to confirm that it is well behaved with remote polling enabled and with remote polling disabled?

It would also be nice to remove the unused label "HEADS" that was mentioned by Kostya.

@ndeloof ndeloof force-pushed the ndeloof:JENKINS-29066 branch from 54d0c5f to f6f0914 Jun 29, 2015

ndeloof and others added some commits Jun 25, 2015

JGit do return some refs (refs/pull/1/merge for sample) we didn’t exp…
…ected during polling, and later call to determineRevisionToBuild won’t consider, resulting in infinite build schedule

@ndeloof ndeloof force-pushed the ndeloof:JENKINS-29066 branch from f6f0914 to 06dc97f Jun 29, 2015

@reviewbybees

This comment has been minimized.

Copy link

reviewbybees commented Jul 1, 2015

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be
reviewed by other CloudBees employees before we seek to have the change accepted. If you want to find out
more about our process please see this note

@stephenc

This comment has been minimized.

Copy link
Member

stephenc commented Jul 6, 2015

🐝

@MarkEWaite

This comment has been minimized.

Copy link

MarkEWaite commented Jul 6, 2015

@ndeloof, based on the reviewbybees process rules, since there has been 1 review (by Stephen) and more than 12 hours since the last commit, is this considered ready to merge?

@KostyaSha

This comment has been minimized.

Copy link
Member

KostyaSha commented Jul 6, 2015

@MarkEWaite as non CB employer you can merge any PR without looking on their 🐝

@MarkEWaite

This comment has been minimized.

Copy link

MarkEWaite commented Jul 6, 2015

@KostyaSha thanks for your note. I know that the process is very careful to describe that I can ignore the CloudBees markers, but I am trying to exercise extra care with the git plugin as we bring it to its next release. I hope we're not many days from the next release, since there are many fixes already merged and verified. I believe this PR needs to be merged and two PR's from Jean Blanchard, then we should be ready for a few days of final test and a release.

@ndeloof

This comment has been minimized.

Copy link
Member Author

ndeloof commented Jul 6, 2015

@MarkEWaite from cloudbees review process PoV this PR indeed is ready to be merged

MarkEWaite added a commit that referenced this pull request Jul 6, 2015

Merge pull request #331 from ndeloof/JENKINS-29066
[JENKINS-29066] potential fix

@MarkEWaite MarkEWaite merged commit 3745d56 into jenkinsci:master Jul 6, 2015

1 check passed

Jenkins This pull request looks good
Details
@@ -518,7 +518,7 @@ private boolean requiresWorkspaceForPolling(EnvVars environment) {
for (GitSCMExtension ext : getExtensions()) {
if (ext.requiresWorkspaceForPolling()) return true;
}
return false;
return getSingleBranch(environment) == null;

This comment has been minimized.

Copy link
@KostyaSha

KostyaSha Sep 8, 2015

Member
    /**
     * If the configuration is such that we are tracking just one branch of one repository
     * return that branch specifier (in the form of something like "origin/master" or a SHA1-hash
     *
     * Otherwise return null.
     */
    private String getSingleBranch(EnvVars env) {
        // if we have multiple branches skip to advanced usecase
        if (getBranches().size() != 1) {
            return null;
        }

Kills polling without workspace for jobs that tracks origin/** and causes infinite builds for cloud slaves.

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.