Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

(GH-494) Support setting of baseline state for each issue #638

Merged
merged 5 commits into from
Jun 24, 2024

Conversation

christianbumann
Copy link
Member

@christianbumann christianbumann commented May 30, 2024

The Sarif report contains the BaselineState of an issue based on the SarifIssueReportFormatSettings.ExistingIssues information. BaseLineState Updated is not supported.

Fixes #494

@christianbumann christianbumann requested a review from a team as a code owner May 30, 2024 10:25
@pascalberger pascalberger self-requested a review May 30, 2024 10:49
@pascalberger pascalberger changed the title Support setting of baseline state for each issue (#494) (GH-494) Support setting of baseline state for each issue May 30, 2024
@christianbumann christianbumann force-pushed the feature/GH-494 branch 4 times, most recently from b9a378b to 16c4cec Compare May 30, 2024 19:14
@pascalberger
Copy link
Member

pascalberger commented May 30, 2024

@pascalberger changes done. Should I add a second property to the settings to define if the IssueComparer should be initialized with true to compare only persistent properties?

@christianbumann See my comment above, with the argument on the IssueComparer ctor it should be possible to support BaselineState.Updated.

@pascalberger
Copy link
Member

We might also think about setting baselineGuid property: https://docs.oasis-open.org/sarif/sarif/v2.0/csprd02/sarif-v2.0-csprd02.html#_Toc10127680

@christianbumann christianbumann changed the title (GH-494) Support setting of baseline state for each issue WIP (GH-494) Support setting of baseline state for each issue Jun 2, 2024
@christianbumann christianbumann force-pushed the feature/GH-494 branch 2 times, most recently from 259c21c to 598b244 Compare June 13, 2024 11:57
@christianbumann christianbumann changed the title WIP (GH-494) Support setting of baseline state for each issue (GH-494) Support setting of baseline state for each issue Jun 13, 2024
@christianbumann
Copy link
Member Author

We might also think about setting baselineGuid property: https://docs.oasis-open.org/sarif/sarif/v2.0/csprd02/sarif-v2.0-csprd02.html#_Toc10127680

BaselineGuid added

@christianbumann
Copy link
Member Author

@pascalberger changes done. Should I add a second property to the settings to define if the IssueComparer should be initialized with true to compare only persistent properties?

@christianbumann See my comment above, with the argument on the IssueComparer ctor it should be possible to support BaselineState.Updated.

Update added

Copy link
Member

@pascalberger pascalberger left a comment

Choose a reason for hiding this comment

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

I fixed a few comment issues and made smaller changes to test cases.

I would like to have another look at the implementation to see if there is another implementation which does not require that much repeated loops through issues lists.

Copy link
Member Author

@christianbumann christianbumann 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, imho on test could be improved and another test could be added.

Christian Bumann and others added 4 commits June 24, 2024 21:45
The Sarif report contains the BaselineState of an issue based on the SarifIssueReportFormatSettings.ExistingIssues information.
BaseLineState Updated is not supported.
Copy link
Member Author

@christianbumann christianbumann left a comment

Choose a reason for hiding this comment

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

Using the new logic is not required if no existing issues are provided. But without existing issues imho the performance loss is very very small - so this would give more security against the new logic. So go for if with the pull request 👍

@christianbumann
Copy link
Member Author

Using the new logic is not required if no existing issues are provided. But without existing issues imho the performance loss is very very small - so this would give more security against the new logic. So go for if with the pull request 👍

The new logic is required to mark the issues as new… (your comment…)

@pascalberger
Copy link
Member

Using the new logic is not required if no existing issues are provided. But without existing issues imho the performance loss is very very small - so this would give more security against the new logic. So go for if with the pull request 👍

@christianbumann BaseLineState should also be set if no existing issues are passed, if a Guid is passed. Consider a first run without any issues, and a second run with 1 issue. In this case there are no existing issues when generating the report for the second run, but BaseLineState should stil lbe New for the newly introduced issue.

@pascalberger pascalberger merged commit dfbca1f into cake-contrib:develop Jun 24, 2024
109 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support setting of baseline state for each issue
2 participants