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

fix: trim leading and trailing newlines off SARIF output #606

Merged
merged 1 commit into from
Oct 24, 2023
Merged

fix: trim leading and trailing newlines off SARIF output #606

merged 1 commit into from
Oct 24, 2023

Conversation

G-Rath
Copy link
Collaborator

@G-Rath G-Rath commented Oct 24, 2023

I don't know a lot about SARIF so maybe these are required but that would be surprising to me whereas trimming these off make it a bit consistent for editors and such.

Obviously if these are required then lets close this PR

@codecov-commenter
Copy link

Codecov Report

Merging #606 (60fe2d5) into main (c00f630) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #606   +/-   ##
=======================================
  Coverage   80.61%   80.61%           
=======================================
  Files          78       78           
  Lines        5334     5334           
=======================================
  Hits         4300     4300           
  Misses        866      866           
  Partials      168      168           
Files Coverage Δ
internal/output/sarif.go 90.42% <100.00%> (ø)

@another-rex another-rex merged commit 32ad454 into google:main Oct 24, 2023
8 checks passed
@G-Rath
Copy link
Collaborator Author

G-Rath commented Oct 24, 2023

image

😞

@G-Rath G-Rath deleted the trim-sarif-output branch October 24, 2023 23:27
@another-rex
Copy link
Collaborator

Oh, did you used to just press the delete branch button?

@G-Rath
Copy link
Collaborator Author

G-Rath commented Oct 24, 2023

Yeah that's what I've always been doing on this repo and e.g. what I did after posting that screenshot - I guess you were right it just doesn't work across forks 🤷

another-rex added a commit to another-rex/osv-scanner that referenced this pull request Oct 26, 2023
* Add name field to sarif rule output (google#600)

Fixes google#599

* fix: trim leading and trailing newlines off SARIF output (google#606)

I don't know a lot about SARIF so maybe these are required but that
would be surprising to me whereas trimming these off make it a bit
consistent for editors and such.

Obviously if these are required then lets close this PR

* Update release pipeline (google#602)

Update the release pipeline to have it be manually triggered, and create
a tag itself if checks and scans are successful.

It looks something like this:

![image](https://github.com/google/osv-scanner/assets/106129829/8258a7d3-3438-46f7-82cd-49c2b4056ecf)

The release doc has been updated to follow this new workflow.

* chore: don't trim trailing whitespace on fixture snapshots (google#608)

This makes it easier to work with for IDEs

* Prepare for 1.4.2 release (google#609)

* Update jekyll feed, for some reason renovatebot can't figure out how to update this

* Fix documentation for github action

* Set upload tag name

---------

Co-authored-by: Gareth Jones <Jones258@Gmail.com>
josieang pushed a commit to josieang/osv-scanner that referenced this pull request Oct 26, 2023
I don't know a lot about SARIF so maybe these are required but that
would be surprising to me whereas trimming these off make it a bit
consistent for editors and such.

Obviously if these are required then lets close this PR
josieang pushed a commit to josieang/osv-scanner that referenced this pull request Oct 27, 2023
I don't know a lot about SARIF so maybe these are required but that
would be surprising to me whereas trimming these off make it a bit
consistent for editors and such.

Obviously if these are required then lets close this PR
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.

3 participants