Improve accuracy of detecting branch names for repos checked out in detached HEAD state#500
Conversation
…etached HEAD state
nikita-tkachenko-datadog
added
the
changelog/Fixed
drodriguezhdez
left a comment
drodriguezhdez
left a comment
There was a problem hiding this comment.
Dropped some comments.
| for (Ref ref : repository.getRefDatabase().getRefs()) { | ||
| String refName = ref.getName(); | ||
| if (Constants.HEAD.equals(refName) || GitUtils.isValidCommitSha(refName)) { | ||
| continue; | ||
| } | ||
| ObjectId refObjectId = ref.getObjectId(); | ||
| if (branch.equals(refObjectId.getName())) { | ||
| return refName; | ||
| matchingBranches.add(normalizeBranch(refName)); | ||
| } | ||
| } | ||
| return null; | ||
| return matchingBranches; |
There was a problem hiding this comment.
This method now is basically iterating over all available refs in the repository. Is this expected?
I foresee issues with this. The number of iterations can be huge depending on the repository.
Also, it seems the method chooseBranch(...) is always getting the first branch in the collection.
Am I missing something?
There was a problem hiding this comment.
It does iterate over refs in the repo, but that was possible before the changes too, depending on where in the list the branch that we're looking for was located.
The method returns the first branch if the collection does not contain the name of the branch that is set in the environment variables (which may or may not be the case, especially with pipelines that do multiple checkouts - multi-branch pipelines, pipelines that use shared libraries, etc).
Anyway, I rewrote the logic a bit to address the comments. Now branch resolution is done inside the callback, and examining the references can be disabled with an env var.
| } else if (branches.size() > 1) { | ||
| // multiple branches point to the checked out commit, | ||
| // and none of them is the same as the branch that is set in the env vars, | ||
| // we are taking a guess at this point | ||
| LOGGER.warning("Build " + buildName + " has multiple Git branches matched: " + branches); | ||
| return branches.iterator().next(); | ||
|
|
||
| } else if (branches.size() == 1) { | ||
| // only one branch points to the checked out commit | ||
| // (also possible that the repo was checked out properly, and not in a detached HEAD state) | ||
| return branches.iterator().next(); |
There was a problem hiding this comment.
If I understood correctly, here we're always returning the first branch (iterator().next()) for both cases branches.size() >= 1. If that's the case why we need to store all branches?
Also, could we simplify this block using >=1? And, should we use List<> instead of Collection<> to use get(0)?
There was a problem hiding this comment.
All branches were needed in order to check if the list contains the branch that is set in the env vars. I removed this method in favour of resolving the branch inside the repo callback, but the logic remained essentially the same.
| final String repoUrl; | ||
| final String defaultBranch; | ||
| final String branch; | ||
| final @Nonnull Collection<String> branches; |
There was a problem hiding this comment.
Why not use List<String> to have more convenient methods for random access (e.g get(0))?
There was a problem hiding this comment.
Rewrote this part
| * It is possible that multiple branches point to the same commit. | ||
| * In which case the logic in this method tries to use a "hint" extracted from environment variables. | ||
| */ | ||
| private static String chooseBranch(@Nonnull Collection<String> branches, @Nullable String branchHint, String buildName) { |
There was a problem hiding this comment.
Any possibility of adding unit tests for this method?
There was a problem hiding this comment.
Moved this logic to the repo callback
drodriguezhdez
left a comment
drodriguezhdez
left a comment
There was a problem hiding this comment.
LGTM. I like the idea of adding the env var to disable the ref search.
Let's document it as well on the docs.
| public static final String GIT_REPOSITORY_URL = "GIT_URL"; | ||
| public static final String GIT_REPOSITORY_URL_ALT = "GIT_URL_1"; | ||
| public static final String GIT_BRANCH = "GIT_BRANCH"; | ||
| public static final String GIT_BRANCH_ALT = "BRANCH_NAME"; |
There was a problem hiding this comment.
Caution
In pull request builds, this can end up being "PR-123" (e.g. for a build on a branch that's part of pull request number 123). In those cases, the fallback env var CHANGE_BRANCH should be preferred over others. The precedence we generally use is def branch = env.CHANGE_BRANCH ?: env.GIT_BRANCH ?: env.BRANCH_NAME (Groovy example; though env.CHANGE_BRANCH ?: env.BRANCH_NAME is generally safe enough). Would you consider updating the logic as such? Thanks!
There was a problem hiding this comment.
Thanks for highlighting this issue! The fix for it is ready and will be released the following week
3 participants
Requirements for Contributing to this repository
What does this PR do?
Improves the accuracy of detecting Git branch names:
BRANCH_NAMEenvironment variable (automatically set by Jenkins for some pipelines) whenGIT_BRANCHis not availableDescription of the Change
Alternate Designs
Possible Drawbacks
Verification Process
Additional Notes
Release Notes
Review checklist (to be filled by reviewers)
changelog/label attached. If applicable it should have thebackward-incompatiblelabel attached.do-not-merge/label attached.kind/andseverity/labels attached at least.