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

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

Merged
merged 13 commits into from May 3, 2024

Conversation

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

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.

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

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

src/upload-lib.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@angelapwen angelapwen left a comment

Choose a reason for hiding this comment

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

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

src/upload-lib.ts Outdated Show resolved Hide resolved
@koesie10
Copy link
Member Author

koesie10 commented May 2, 2024

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.

Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

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

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

src/upload-lib.ts Outdated Show resolved Hide resolved
src/upload-lib.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

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

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

@koesie10
Copy link
Member Author

koesie10 commented May 2, 2024

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

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

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.

LGTM. Thanks for addressing my comments.

Copy link
Contributor

@angelapwen angelapwen left a comment

Choose a reason for hiding this comment

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

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

@koesie10 koesie10 merged commit 93b8232 into main May 3, 2024
320 checks passed
@koesie10 koesie10 deleted the koesie10/deprecate-merge branch May 3, 2024 08:23
@github-actions github-actions bot mentioned this pull request May 8, 2024
8 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.

None yet

4 participants