Skip to content
This repository has been archived by the owner on Oct 30, 2024. It is now read-only.

adds metadata to sarif result #486

Merged
merged 4 commits into from
Jan 26, 2023

Conversation

dani-santos-code
Copy link
Contributor

@dani-santos-code dani-santos-code commented Oct 5, 2022

This is to add the Metadata field/info to the sarif. We currently display this info on the CLI. but it was missing on the sarif. Here's an example of what it will look like on Github UI:

Screenshot 2023-01-23 at 5 45 14 PM

@dani-santos-code dani-santos-code force-pushed the ds/adds-metadata-to-sarif-result branch 5 times, most recently from 110288b to 8acd3c4 Compare October 5, 2022 18:26
@dani-santos-code dani-santos-code force-pushed the ds/adds-metadata-to-sarif-result branch from 8acd3c4 to 84b4e25 Compare October 5, 2022 18:29
@dani-santos-code dani-santos-code force-pushed the ds/adds-metadata-to-sarif-result branch from 84b4e25 to fc49af1 Compare October 5, 2022 18:39
@dani-santos-code dani-santos-code marked this pull request as ready for review January 16, 2023 16:45
thepwagner
thepwagner previously approved these changes Jan 17, 2023
Copy link
Contributor

@thepwagner thepwagner 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 err vs jsonError is worth fixing.

Other comments stem from me not fully understanding what Metadata will contain, and so getting paranoid about edge cases. Those are safe to ignore!

internal/sarif/sarif.go Outdated Show resolved Hide resolved
internal/sarif/sarif.go Outdated Show resolved Hide resolved
internal/sarif/sarif.go Outdated Show resolved Hide resolved
Co-authored-by: Pete Wagner <1559510+thepwagner@users.noreply.github.com>
@dani-santos-code dani-santos-code force-pushed the ds/adds-metadata-to-sarif-result branch from c5f3234 to 2ad506d Compare January 23, 2023 19:32
@dani-santos-code dani-santos-code changed the title adds metadata to result adds metadata to sarif result Jan 23, 2023
Copy link
Contributor

@thepwagner thepwagner left a comment

Choose a reason for hiding this comment

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

No blockers, just an optimization idea.

formattedMap[k] = v
}

metadata, jsonErr := json.Marshal(formattedMap)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: we don't use this value if len(formattedMap) > 0 below.

While it's pretty cheap to render {}, we could just skip that by moving the map construction and JSON serialization to be within the guard.
I checked and kubeaudit.Metadata is a typedef of map[string]string - could we just use that directly to skip all of the map construction?

var metadataTxt string
if len(result.Metadata) > 0 {
    metadata, jsonErr := json.Marshal(result.Metadata)
    ...
    metadataTxt = fmt.Sprintf("Metadata: %s\n", string(metadata))
}

Nobody will notice the like 8 bytes of memory that we save, but we will know. 😎

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah! excellent point! applied here

@dani-santos-code dani-santos-code merged commit ad40a0c into main Jan 26, 2023
@dani-santos-code dani-santos-code deleted the ds/adds-metadata-to-sarif-result branch January 26, 2023 19:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants