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

Add deprecation warning for merging SARIF files with non-unique categories by koesie10 · Pull Request #2265 · github/codeql-action · GitHub

/ codeql-action Public

Add deprecation warning for merging SARIF files with non-unique categories - #2265

Merged
koesie10 merged 13 commits into
mainfrom
koesie10/deprecate-merge
May 3, 2024
ConversationCommits (13)ChecksFiles changed
Merged

Add deprecation warning for merging SARIF files with non-unique categories#2265
koesie10 merged 13 commits into
mainfrom
koesie10/deprecate-merge

Conversation

koesie10 commented Apr 26, 2024

Copy link
Copy Markdown
Member

This adds a deprecation warning for merging SARIF files with non-unique categories. This is behind a feature flag.

These are the criteria for showing the deprecation warning:

  • The feature flag is enabled
  • If on GHES: GHES version >= 3.14.0
  • The deprecation warning hasn't been shown in the same run
  • Non-unique categories are found

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.

koesie10 marked this pull request as ready for review May 1, 2024 08:07
koesie10 requested a review from a team as a code owner May 1, 2024 08:07
aeisenberg reviewed May 1, 2024
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

I think this is good and should work. Two minor comments.

However, I think we should add some tests here. The logic is a bit complex and it would be good to make sure the messages are emitted only when we expect them to be.

Comment thread src/upload-lib.ts Outdated
)
) {
logger.warning(
`Uploading multiple CodeQL runs with the same category is deprecated ${deprecationWarningMessage}. Please update your CodeQL CLI version or update your workflow to set a distinct category for each CodeQL run. For more information, see https://github.blog/changelog/2024-05-06-code-scanning-will-stop-combining-runs-deprecation-notice`,

aeisenberg May 1, 2024

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

Is this the same message as above? If so, probably best to extract to a variable so we don't accidentally change one but not the other.

koesie10 May 2, 2024

Copy link
Copy Markdown
Member 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

No, the message is not exactly the same. I'll extract the last part ("For more information, see ..."), but the other parts of the message are different. The first message is for third-party tools, while the second message is more specific to CodeQL-only uploads.

Comment thread src/upload-lib.ts Outdated
angelapwen reviewed May 1, 2024
View reviewed changes

angelapwen 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

Makes sense to me! Just a small comment on top of Andrew's.

Comment thread src/upload-lib.ts Outdated

koesie10 commented May 2, 2024

Copy link
Copy Markdown
Member Author

Thanks for the reviews!

However, I think we should add some tests here. The logic is a bit complex and it would be good to make sure the messages are emitted only when we expect them to be.

I've added tests for the (renamed) shouldShowCombineSarifFilesDeprecationWarning function which tests that this function only returns true when all conditions are met. Since this is called before showing the message, this should ensure that we never show the message inadvertently.

henrymercer reviewed May 2, 2024
View reviewed changes

henrymercer 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

Looks good, just a comment about the warning for CodeQL.

Comment thread src/upload-lib.ts Outdated
Comment thread src/upload-lib.ts
henrymercer approved these changes May 2, 2024
View reviewed changes

henrymercer 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

Looks good to me to merge once the changelog post is out.

koesie10 commented May 2, 2024

Copy link
Copy Markdown
Member Author

Looks good to me to merge once the changelog post is out.

I think we can actually merge this before the changelog post is out since the deprecation message is behind a feature flag. Does that make sense, or do you think it would be better to wait?

henrymercer commented May 2, 2024

Copy link
Copy Markdown
Contributor

Ah perfect, just make sure the changelog's out before you start rolling out the feature flag then! Thanks!

aeisenberg approved these changes May 2, 2024
View reviewed changes

aeisenberg left a comment •
edited
Loading

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

LGTM. Thanks for addressing my comments.

angelapwen approved these changes May 2, 2024
View reviewed changes

angelapwen 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

Not that you needed this third approval but providing it anyway 😆

koesie10 merged commit 93b8232 into main May 3, 2024
koesie10 deleted the koesie10/deprecate-merge branch May 3, 2024 08:23
github-actions Bot mentioned this pull request May 8, 2024
Merged
8 tasks
henrymercer mentioned this pull request May 8, 2024
Merged
8 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
henrymercer henrymercer approved these changes
+1 more reviewer
angelapwen angelapwen approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL