-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Retrieve full name matches over hazy short hash matches #1293
base: master
Are you sure you want to change the base?
Conversation
… need for this feature
… to do with my dynamic test generator (not grabbing the right tag head)
… with dynamic test creation
… need for this feature
… to do with my dynamic test generator (not grabbing the right tag head)
… with dynamic test creation
…etrieve-full-name-first # Conflicts: # src/test/java/jenkins/plugins/git/AbstractGitSCMSourceTest.java
Will this get merged in? I could really use it, the plugin is not select the correct git ref and this looks like it fixes it! |
Not immediately. It has not been reviewed. I did a cursory review and the change looks like a very good start. It includes an automated test (very good). However, it also makes changes that are contrary to the guidelines in the contributing guide (changing white space unnecessarily) and contrary to the guidelines in the SCM API code style guidelines (wild card imports). It will need changes before it is merged. I have several other reviews that are in my queue ahead of this review. You could help by installing the incremental build of the plugin in your environment and confirming that it resolves the issue you are seeing and does not have any unexpected side effects. If you use configuration as code, you can insert this incremental build into your plugins.txt file with the line:
|
pom.xml
Outdated
</dependency> | ||
<dependency> | ||
<groupId>org.jenkins-ci.plugins</groupId> | ||
<artifactId>git</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this here...twice?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. The comment on the commit indicates that it was intended to be temporary. I've removed it, merged the master branch, and pushed the change
JENKINS-62592 - Return exact matches over fuzzy short hash matches while retrieving revisions.
Not a large change, but this issue addresses the problem arisen by the associated ticket (https://issues.jenkins.io/browse/JENKINS-62592). Commonly, we have seen due to our tagging method/branch name method that we will clash with the hazy short hash revision matching functionality within the AbstractGitSCMSource.retrieve method.
Checklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. This is simply a reminder of what we are going to look for before merging your code. If a checkbox or line does not apply to this pull request, delete it. We prefer all checkboxes to be checked before a pull request is mergedTypes of changes
What types of changes does your code introduce? Put an
x
in the boxes that apply. Delete the items in the list that do not applyFurther comments
Change can be summarized and seen in the associated test - instead of returning null when we first clash based on short hash, we store that we have noticed a collision and continue on. This way, if we find a full match, we can return what we see as the expected revision to retrieve.
Let me know if there are any comments or concerns that I am over looking and I look forward to helping in any way I can in this project! Thanks!