-
Notifications
You must be signed in to change notification settings - Fork 4
NEW @W-17159591@ Added version property to output #142
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
Conversation
| tool: { | ||
| driver: { | ||
| name: engineRunResults.getEngineName(), | ||
| semanticVersion: engineRunResults.getEngineVersion(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SARIF supports both semanticVersion and version. semanticVersion should be used if the version is known to conform to Semantic Version 2 schema. Since all of the versions we're returning are pulled directly from the package.json, they therefore definitionally conform to the schema.
My assumption is that external plugin authors will also return something that resembles a SemVer, and thus this should be valid for them too. If that assumption is incorrect, I can just change to the version property trivially.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the validator https://sarifweb.azurewebsites.net/Validation validate with semanticVersion supplied but not version?
Also yeah, not sure how things will work out given that we are not forcing SemVer in the engine api... so I wonder if we just use "version". Is "version" what we used with v4?
Alternatively, we could just see if it is of a specific format (X.Y.Z) and if so, then use semanticVersion and if not then just fall back to version.
| tool: { | ||
| driver: { | ||
| name: engineRunResults.getEngineName(), | ||
| semanticVersion: engineRunResults.getEngineVersion(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the validator https://sarifweb.azurewebsites.net/Validation validate with semanticVersion supplied but not version?
Also yeah, not sure how things will work out given that we are not forcing SemVer in the engine api... so I wonder if we just use "version". Is "version" what we used with v4?
Alternatively, we could just see if it is of a specific format (X.Y.Z) and if so, then use semanticVersion and if not then just fall back to version.
| const versionsNode: xmlbuilder.XMLElement = resultsNode.node('versions'); | ||
| const engineNames: string[] = results.getEngineNames(); | ||
| for (const engineName of engineNames) { | ||
| versionsNode.node(engineName).text(results.getEngineRunResults(engineName).getEngineVersion()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm... This would be the first time we would be generating dynamic tags. Thankfully tag names can contain hyphens. That is, we would have <versions><pmd>X.Y.Z</pmd><retire-js>X.Y.Z</retire-js></versions> for example.
This is fine but we have to keep in mind that if we ever publish an xsd associated with these dynamic tags, we'd have to use something like <xs:any minOccurs="0" maxOccurs="unbounded"/> when documenting the tags under the versions tag.
No description provided.