FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Declare the merge base as base for code scanning comparisons by cannist · Pull Request #858 · github/codeql-action · GitHub

/ codeql-action Public

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

Merged
cannist merged 4 commits into
mainfrom
use-better-base-sha
Feb 4, 2022
ConversationCommits (4)ChecksFiles changed
Merged

Declare the merge base as base for code scanning comparisons#858
cannist merged 4 commits into
mainfrom
use-better-base-sha

Conversation

cannist commented Dec 17, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 force-pushed the use-better-base-sha branch from 4d28f0c to ec0b3ae Compare February 1, 2022 15:40
cannist requested a review from rneatherway February 1, 2022 15:44
cannist marked this pull request as ready for review February 1, 2022 15:45
cannist requested a review from a team as a code owner February 1, 2022 15:45
aeisenberg reviewed Feb 1, 2022
View reviewed changes

aeisenberg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

There's also a potential conflict with #889.

Comment thread src/actions-util.ts
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);
}

aeisenberg Feb 1, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nice trick to get the merge base.

Comment thread src/upload-lib.ts
@@ -313,15 +314,28 @@ export function buildPayload(
gitHubVersion.type !== util.GitHubVariant.GHES ||
semver.satisfies(gitHubVersion.version, `>=3.1`)

aeisenberg Feb 1, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Do we need to change the satisfies here? Will GHES < 3.4 accept the new mergeBaseCommitOid parameter?

cannist Feb 1, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Feb 2, 2022

Copy link
Copy Markdown
Contributor

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

rneatherway commented Feb 2, 2022

Copy link
Copy Markdown
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 commented Feb 2, 2022

Copy link
Copy Markdown
Contributor Author

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 commented Feb 2, 2022

Copy link
Copy Markdown
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 commented Feb 3, 2022

Copy link
Copy Markdown
Contributor Author

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 commented Feb 3, 2022

Copy link
Copy Markdown
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 commented Feb 4, 2022

Copy link
Copy Markdown
Contributor

Feel free to merge after the conflict has been addressed.

cannist commented Feb 4, 2022

Copy link
Copy Markdown
Contributor Author

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 merged commit 904d0ac into main Feb 4, 2022
cannist deleted the use-better-base-sha branch February 4, 2022 12:37
github-actions Bot mentioned this pull request Feb 7, 2022
Merged
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

aeisenberg aeisenberg approved these changes
rneatherway Awaiting requested review from rneatherway

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL