-
Notifications
You must be signed in to change notification settings - Fork 309
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
Declare the merge base as base for code scanning comparisons #858
Conversation
4d28f0c
to
ec0b3ae
Compare
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.
There's also a potential conflict with #889.
if (data.startsWith("commit ") && commitOid === "") { | ||
commitOid = data.substring(7); | ||
} else if (data.startsWith("parent ")) { | ||
if (baseOid === "") { | ||
baseOid = data.substring(7); | ||
} else if (headOid === "") { | ||
headOid = data.substring(7); | ||
} |
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.
Nice trick to get the merge base.
@@ -313,15 +314,28 @@ export function buildPayload( | |||
gitHubVersion.type !== util.GitHubVariant.GHES || | |||
semver.satisfies(gitHubVersion.version, `>=3.1`) |
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.
Do we need to change the satisfies here? Will GHES < 3.4 accept the new mergeBaseCommitOid
parameter?
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.
There is no new parameter on the dotcom side. This is only about computing a better value for the existing parameter base_sha
in some cases. mergeBaseCommitOid
is passed into the buildPayload
function in this library which then selects it as the base_sha
or not.
LGTM. It's a shame that it's a little bit complicated to get something as simple as the first parent, especially when it's trivially available on the server side. However, I trust your judgement if you prefer to do it here. I'll leave sign-off to Andrew when he's happy. |
I've merged in master and resolved the conflicts by running |
I think it's fine if there is no changelog. This is an internal change. However, I thought of something last night as I was falling asleep. The new input to specify the |
Hmm, I thought that PR was closed? ... Ah, I see there was a substitute #904 I think I already have the case covered. IMO I think there is a question of what we should declare as base when the upload |
Thanks for the explanation. I think it all makes sense. What is the danger of using a different merge base? Should we be adding another new input that accepts a merge base if and only if a sha and ref are specified? |
Feel free to merge after the conflict has been addressed. |
We're only talking about the case where the action is triggered by the This could become a problem if the ref is set to indicate a different PR. Then using the target branch and base sha data of the PR in whose context the action is running is just wrong. |
When uploading results for a pull request we tell code scanning against what other commit the results should be compared. So far we've always used the pull request base. However, when we've analyzed the merge of the feature branch and the default branch, it is much more accurate to use the head of the target branch in the comparison.
It was surprisingly difficult to determine the merge base since the default clone in actions is shallow and does not contain the merge's parents. That means the more direct approaches of getting parent data, e.g.,
git rev-parse HEAD^1
, fail. The only way I could find was to dogit show --format=raw
and then parse the output.Open question:
Merge / deployment checklist