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-48061] Partial fix for the easy case of non-head revision in history of branch #561

Merged
merged 1 commit into from Dec 7, 2017

Conversation

Projects
None yet
4 participants
@stephenc
Copy link
Member

commented Dec 6, 2017

See JENKINS-48061

Supercedes #557

This is only a partial fix, the two additional ignored test cases are required for the full fix

@stephenc stephenc requested review from jglick and MarkEWaite Dec 6, 2017

@jglick

jglick approved these changes Dec 7, 2017

Copy link
Member

left a comment

Only vaguely follow.

@stephenc

This comment has been minimized.

Copy link
Member Author

commented Dec 7, 2017

May as well merge this half as it should help at least 80%

@stephenc stephenc merged commit 828ca74 into jenkinsci:master Dec 7, 2017

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details

@stephenc stephenc deleted the stephenc:jenkins-48061-partial branch Dec 7, 2017

@mbelang

This comment has been minimized.

Copy link

commented Dec 15, 2017

@stephenc When do you plan the next release that will include that fix?

@MarkEWaite

This comment has been minimized.

Copy link

commented Dec 15, 2017

@mbelang have you confirmed that the latest build resolves the issue for you? I didn't see any change in the cases I was testing.

@mbelang

This comment has been minimized.

Copy link

commented Dec 15, 2017

No I didn't. Are you saying that it is not really fixed?

@MarkEWaite

This comment has been minimized.

Copy link

commented Dec 15, 2017

@stephenc said that it is 80%. I would love to have you confirm that your failure case is included in that 80%. That would indicate that there is some flaw in my testing (and give further confirmation that the change helps).

@mbelang

This comment has been minimized.

Copy link

commented Dec 15, 2017

I'll give it a try when I get a chance next week and get back to you.

@stephenc

This comment has been minimized.

Copy link
Member Author

commented Dec 15, 2017

There are 3 tests in this PR, 2 of which are skipped because they represent cases that this partial fix does not address.

My guesstimate is that ~80% of people should match the first test case... but I have no evidence as to the actual %

@mbelang

This comment has been minimized.

Copy link

commented Jan 12, 2018

@stephenc Sorry for the delay but yeah our usecase fall under the ~80%. Your partial fix is good for us.

Thx

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.