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

Declare the merge base as base for code scanning comparisons #858

Merged
merged 4 commits into from
Feb 4, 2022

Conversation

cannist
Copy link
Contributor

@cannist cannist commented Dec 17, 2021

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 do git show --format=raw and then parse the output.

Open question:

  • Does this need a changelog entry? It is very much behind the scenes.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

@cannist cannist marked this pull request as ready for review February 1, 2022 15:45
@cannist cannist requested a review from a team as a code owner February 1, 2022 15:45
Copy link
Contributor

@aeisenberg aeisenberg left a 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.

Comment on lines +118 to +125
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);
}
Copy link
Contributor

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`)
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@aeisenberg
Copy link
Contributor

#889 was closed. It's #902 instead that is making changes in the same general area.

@rneatherway
Copy link
Contributor

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.

@cannist
Copy link
Contributor Author

cannist commented Feb 2, 2022

I've merged in master and resolved the conflicts by running npm run build. Are we happy with this not having a changelog entry?

@aeisenberg
Copy link
Contributor

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 ref and sha for analysis will conflict here semantically. determineMergeBaseCommitOid should return undefined if the ref or sha inputs are defined.

@cannist
Copy link
Contributor Author

cannist commented Feb 3, 2022

However, I thought of something last night as I was falling asleep. The new input to specify the ref and sha for analysis will conflict here semantically. determineMergeBaseCommitOid should return undefined if the ref or sha inputs are defined.

Hmm, I thought that PR was closed? ... Ah, I see there was a substitute #904

I think I already have the case covered. IMO determineMergeBaseCommitOid should not act differently when these inputs are given. Its purpose is to determine the merge base when we're run in the context of a pull request. We should not use its return value when we're not uploading a result for the merge, but that decision should be made by the upload logic, not determineMergeBaseCommitOid. It is covered here.

I think there is a question of what we should declare as base when the upload ref/sha has nothing to do with the context the action is running in. Currently it will still use the previous behaviour of using the PR base. But since that has nothing to do with my change I think it might better be addressed separately.

@aeisenberg
Copy link
Contributor

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?

@aeisenberg
Copy link
Contributor

Feel free to merge after the conflict has been addressed.

@cannist
Copy link
Contributor Author

cannist commented Feb 4, 2022

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?

We're only talking about the case where the action is triggered by the pull_request event. In that case the upload sets the base_ref and base_sha parameters to tell code scanning what the results should best be compared to. Code scanning will only make use of that info if the ref looks like a PR ref, i.e. it is of the form refs/pull/N/merge or refs/pull/N/head to compute and report new and fixed alerts on the PR.

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.
It's unlikely that you'd manually set the ref to some other PR ref, though. I do not think it makes sense to expose the base_* fields as input. We should just not populate them if the ref (and perhaps sha) deviates from the one that can be observed in the context.

@cannist cannist merged commit 904d0ac into main Feb 4, 2022
@cannist cannist deleted the use-better-base-sha branch February 4, 2022 12:37
@github-actions github-actions bot mentioned this pull request Feb 7, 2022
5 tasks
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