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

feat: add $schema to outputted files #471

Conversation

eoinobrien
Copy link
Contributor

@eoinobrien eoinobrien commented Nov 5, 2020

Description of changes

Add's a $schema value to the outputted Sarif file.
Some places, in particular GitHub require a $schema field to be populated

This code adds that schema url using the value from the spec. Tests have been updated to include the field.

I have validated that this change is inline with the Sarif spec

A sarifLog object MAY contain a property named $schema whose value is a string containing an absolute URI from which a JSON schema document [JSCHEMA01] describing the version of the SARIF format to which this log file conforms can be obtained.

Please let me know if this should be treated as a breaking change as the outputs have changed.

Pull request checklist

  • PR title respects Conventional Commits (starts with fix:, feat:, etc, and is suitable for user-facing release notes)
  • PR contains no breaking changes, OR description of both PR and final merge commit starts with BREAKING CHANGE:
  • [n/a] (if applicable) Addresses issue: #0000
  • Added relevant unit tests for your changes
  • Ran yarn precheckin
  • Verified code coverage for the changes made

@eoinobrien eoinobrien requested a review from a team as a code owner November 5, 2020 02:30
@dbjorge dbjorge changed the title fix: Add $schema to outputted files feat: add $schema to outputted files Nov 5, 2020
Copy link
Contributor

@dbjorge dbjorge left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! This looks good to me. @peterdur and I discussed and we think we'd like to treat this as a non-breaking feature; I've update the PR title accordingly.

We feel a little reluctant about using the master branch of the sarif-spec repo rather than pinning to a specific commit/tag, since we've been impacted in the past by that repo merging breaking changes without incrementing versions in filenames. But we think consistency with the spec's recommendation and the schema's $id property is more important, so we'll go with this as-is.

@dbjorge dbjorge merged commit f305c9c into microsoft:master Nov 5, 2020
@dbjorge
Copy link
Contributor

dbjorge commented Nov 5, 2020

This has been released in v2.6.0

@ada-cat
Copy link
Collaborator

ada-cat commented Nov 5, 2020

🎉 This PR is included in version 2.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@eoinobrien eoinobrien deleted the eoinobrien/add-schema-to-outputted-files branch November 6, 2020 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants