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

Fix identity comparison for GitHubSCMSource #218

Merged
merged 1 commit into from
Apr 16, 2019

Conversation

bitwiseman
Copy link
Contributor

@bitwiseman bitwiseman commented Apr 16, 2019

When scanning for new runs, PullRequestSCMRevision instances are compared. While technically one combination of
base and pull commit produce the same merge, if we detect a new merge commit we should still treat that as a new revision.

For example:
Before this change, if a user closes a PR (unmerged) and then reopenes it, a repository scan would not treat the PR as changed.

After this change, a closing, reopening, and then scan will result in change being detected with a message like this (reformatted for clarity):

Changes detected: PR-3-merge (
58caa961c3fc5d68268e0d860670041b97cc86b0+edb61773c0185852a5d5c7afc17a924fee730363 (7c907cc0d0eaf15f2b84d030e23f5fd96e1b59d4) ->
58caa961c3fc5d68268e0d860670041b97cc86b0+edb61773c0185852a5d5c7afc17a924fee730363 (e0dd5f238a71951e6a48dc01443fb624a44ff278))

When scanning for new runs, PullRequestSCMRevision instances are compared.  While technically one combination of
base and pull commit produce the same merge, if we detect a new merge commit we should still treat that as a new revision.

For example:
Before this change, if a user closes a PR (unmerged) and then reopenes it, a repository scan would not treat the PR as changed.

After this change, a closing, reopening, and then scan will result in change being detected with a message like this:

Changes detected: PR-3-merge (
58caa961c3fc5d68268e0d860670041b97cc86b0+edb61773c0185852a5d5c7afc17a924fee730363 (7c907cc0d0eaf15f2b84d030e23f5fd96e1b59d4) ->
58caa961c3fc5d68268e0d860670041b97cc86b0+edb61773c0185852a5d5c7afc17a924fee730363 (e0dd5f238a71951e6a48dc01443fb624a44ff278))
@bitwiseman bitwiseman merged commit ce4f584 into jenkinsci:master Apr 16, 2019
@bitwiseman bitwiseman deleted the no-clone-id branch April 16, 2019 19:57
@awiddersheim
Copy link

I followed a rabbit hole to this PR. I'm wondering if it is causing this issue:

https://issues.jenkins-ci.org/browse/JENKINS-57583

This PR changes the equivalent() method to now take into account the GitHub generated merge commit hash:

https://github.com/jenkinsci/github-branch-source-plugin/pull/218/files#diff-ae61dccc33102b887acf9bf8b02ec9cdR124

The underlying base class loosely seems to indicate that the target should not be considered when doing equivalent() but by considering the merge commit hash this is no longer the case.

https://github.com/jenkinsci/scm-api-plugin/blob/master/src/main/java/jenkins/scm/api/mixin/ChangeRequestSCMRevision.java#L84-L86

This is then causing the code below to no longer work.

https://github.com/jenkinsci/basic-branch-build-strategies-plugin/blob/master/src/main/java/jenkins/branch/buildstrategies/basic/ChangeRequestBuildStrategyImpl.java#L129-L134

I haven't fully vetted all of this so I could be totally off base but would appreciate a second pair of eyes on this.

@bitwiseman
Copy link
Contributor Author

@awiddersheim
This is an excellent analysis.

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.

3 participants