SarifLogger now emits an artifacts table entry if artifactLocation is not null for tool configuration and tool execution notifications.
#2437
Conversation
This file contains 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
src/ReleaseHistory.md
Outdated
| @@ -5,6 +5,7 @@ | |||
| * BREAKING: `AnalyzeCommandBase` previously persisted all scan target artifacts to SARIF logs rather than only persisting artifacts referenced by an analysis result, when an option to persist hashes, text file or binary information was set. `MultithreadedAnalyzeCommandBase` previously persisted all scan targets artifacts to SARIF logs in cases when hash insertion was eenabled rather than only persisting artifacts referenced by an analysis result. [#2433](https://github.com/microsoft/sarif-sdk/pull/2433) | |||
| * BUGFIX: Adjust Json Serialization field order for ReportingDescriptor and skip emit empty AutomationDetails node. [#2420](https://github.com/microsoft/sarif-sdk/pull/2420) | |||
| * BREAKING: Fix `InvalidOperationException` when using PropertiesDictionary in a multithreaded application, and remove `[Serializable]` from it. Now use of BinaryFormatter on it will result in `SerializationException`: Type `PropertiesDictionary` is not marked as serializable. [#2415](https://github.com/microsoft/sarif-sdk/pull/2415) | |||
| * BUGFIX: Notifications should emit an artifact if artifactLocation is not null. [#2437](https://github.com/microsoft/sarif-sdk/pull/2437) | |||
src/ReleaseHistory.md
Outdated
| @@ -5,6 +5,7 @@ | |||
| * BREAKING: `AnalyzeCommandBase` previously persisted all scan target artifacts to SARIF logs rather than only persisting artifacts referenced by an analysis result, when an option to persist hashes, text file or binary information was set. `MultithreadedAnalyzeCommandBase` previously persisted all scan targets artifacts to SARIF logs in cases when hash insertion was eenabled rather than only persisting artifacts referenced by an analysis result. [#2433](https://github.com/microsoft/sarif-sdk/pull/2433) | |||
| * BUGFIX: Adjust Json Serialization field order for ReportingDescriptor and skip emit empty AutomationDetails node. [#2420](https://github.com/microsoft/sarif-sdk/pull/2420) | |||
| * BREAKING: Fix `InvalidOperationException` when using PropertiesDictionary in a multithreaded application, and remove `[Serializable]` from it. Now use of BinaryFormatter on it will result in `SerializationException`: Type `PropertiesDictionary` is not marked as serializable. [#2415](https://github.com/microsoft/sarif-sdk/pull/2415) | |||
| * BUGFIX: Notifications should emit an artifact if artifactLocation is not null. [#2437](https://github.com/microsoft/sarif-sdk/pull/2437) | |||
There was a problem hiding this comment.
Notifications should emit an artifact if artifactLocation is not null
You didn't document what type's behavior changed, it's SarifLogger. You need this information for customers to assess whether the change impacts them.
SarifLogger now emits an artifacts table entry is artifactLocation is not null for tool configuration and tool execution notifications. #Resolved
There was a problem hiding this comment.
Just updated!
thanks for the suggestion!
eddynaka
changed the title
SarifLogger now emits an artifacts table entry if artifactLocation is not null for tool configuration and tool execution notifications.
eddynaka
deleted the
users/ednakamu/fixing-artifacts-generation-tool-notification
branch
yongyan-gh
pushed a commit
that referenced
this pull request
Feb 8, 2022
…` is not null for tool configuration and tool execution notifications. (#2437) * Fixing artifacts generation when logging notifications * Updating release history. * Updating ReleaseHistory
michaelcfanning
added a commit
that referenced
this pull request
Mar 2, 2022
) * Add new visitor to get deterministic SARIF log by sorting results * Fix dotnet format issue * updating format * remove unnecessary using Format & minor fixes * Add Run Comparer to support sorting logs with multiple runs. * Add command argument unit tests fix dotnet format * use ContainsKey to avoid allocating variable * Fixing `AnalyzeCommandBase` and `MultithreadedAnalyzeCommandBase` artifacts generation (#2433) * Fixing multithreaded artifacts generation * Fixing tests and flags * Loosing precision. * Applying fix for AnalyzeCommandBase * Enabling tests * Updating test case and release history * Creating const to prevent magical numbers everywhere * Rebaselining tests * Creating Artifacts flag to keep previous behavior * Addressing PR feedback. * Rollback changes * Update SARIF2012.ProvideRuleProperties_Invalid.sarif * updating back * Ordering deprecated names * `SarifLogger` now emits an artifacts table entry if `artifactLocation` is not null for tool configuration and tool execution notifications. (#2437) * Fixing artifacts generation when logging notifications * Updating release history. * Updating ReleaseHistory * Fix `ArgumentException` when recurse is enabled and two file target specifiers generates the same file paths (#2438) * Fixing ArgumentException when passing two filePaths that generates duplicated file analysis * Fixing dotnet-format issues and updating releasehistory * Removing comments * Addressing PR feedback * Addressing PR feedback * Addressing PR review issues Add suppression support (#2435) * Add suppression support * Add incompatibility check and make suppressions non-null Co-authored-by: Eddy Nakamura <[email protected]> Update releasehistory fix couple test cases * Fix issues in PR review * Add xml comments * Fix test issues * fix dotnet format * Addressing review feedbacks * Fix tests * Update extension methods names * Change xml doc comments to normal comments Co-authored-by: Eddy Nakamura <[email protected]> Co-authored-by: Michael C. Fanning <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
After we merged this PR when we log a notification, we stopped it from logging to the artifacts. But, this is a bug, since a notification is also a result and that result might have an
artifactLocationwhich can point to the artifacts table.Test
Let's create notifications with/without locations and check if the artifacts table is being updated.