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

Provide full metadata objects for notifications #311

Closed
michaelcfanning opened this issue Jan 21, 2019 · 4 comments
Closed

Provide full metadata objects for notifications #311

michaelcfanning opened this issue Jan 21, 2019 · 4 comments
Labels
design-approved The TC approved the design and I can write the change draft e-ballot e-ballot-3 enhancement impact-breaks-consumers impact-breaks-producers merged Changes merged into provisional draft. p1 Priority 1 issue to close resolved-fixed tc-34

Comments

@michaelcfanning
Copy link
Contributor

michaelcfanning commented Jan 21, 2019

Some tool-specific messages may warrant their own descriptive objects as we have for rules. The reason is that various notifications are commonly occurring adverse conditions that need to be documented and understood as part of a standard support scenario.

A canonical example: 'Rule {0} raised an unhandled exception and was disabled during analysis (which may be incomplete. To temporarily disable this check, pass --disable:{1} on the command-line.'

This string documents an adverse condition that may show up in log files. It therefore may warrant its own help topic, descriptive text, etc. It is helpful for this descriptive information to be documented in an exhaustive SARIF log file that contains a snapshot of all possible user-facing strings.

[Note that this message also demonstrates why a notification may provide both a ruleId and a ruleIndex (pointing into a rule descriptive object). In this case, the rule id is used as arg {1} to form an argument to disable a check by its id. The underlying notification.ruleIndex is used to look-up the rule's friendly rule.name value to pass as a readable identifier in arg {0}].

Simplest idea might be to generalize resources.rules to a more general name that covers descriptors for both notifications and rules. run.resources.messageDescriptors. result.ruleIndex would then represent a rule descriptor index (which could be the name, though long).

A second strategy might be to define a run.resources.notifications property. the elements of this array (and resources.rules) could be the same underlying thing. The arrays would allow for separation of purpose (otherwise we might need an enum on each descriptor to document whether it is a notification or rule descriptor).

Another thinking scenario to serve as an exampl: pdb loading. Many rules may depend on PDBs in order to get access to sufficient data to perform analysis. It is clear that a single help topic would make sense for resolving this adverse condition. According to SARIF, this message s/be a notification (an error that indicates the analysis is not properly configured to run, because the PDBs are missing or because the symbol server was not properly configured on the command-line). Another case for a notification descriptor: a common format string is not sufficient.
@lgolding

@michaelcfanning michaelcfanning added the p1 Priority 1 issue to close label Jan 24, 2019
@michaelcfanning
Copy link
Contributor Author

michaelcfanning commented Jan 24, 2019

Proposal, define a notification metadata and rule metadata object
tool.notificationMetadata
tool.ruleMetadata
run.ruleConfiguration

@michaelcfanning michaelcfanning added the design-approved The TC approved the design and I can write the change draft label Jan 24, 2019
@ghost ghost added discussion-ongoing and removed design-approved The TC approved the design and I can write the change draft labels Jan 28, 2019
@michaelcfanning
Copy link
Contributor Author

We also require tool.globalMessageStrings or simply tool.messageStrings to retain the possibility of sharing strings across rules & notifications. @lgolding FYI.

@michaelcfanning
Copy link
Contributor Author

michaelcfanning commented Feb 8, 2019

EBALLOT PROPOSAL

Provide a unified metadata model for tool notifications as well as tool analysis notifications (rendered as run.results). Many tool notifications need to be documented and handled in ways similar to literal analysis results.

API IMPACT

  • Rename the rule object to reportingDescriptor.
  • Move the rules array (now an array reportingDescriptor objects) out of run.resources and into toolComponent.ruleDescriptors.
  • Move the messageStrings array out of run.resources and into toolComponent.globalMessageStrings.
  • Introduce toolComponent.notificationDescriptors, also an array of reportingDescriptor objects.
  • Remove the run.resources property and the resources object.
  • Rename the ruleConfiguration object to reportingConfiguration.
  • Rename the rule.configuration property to reportingDescriptor.defaultConfiguration.
  • Rename ruleConfiguration.defaultLevel and ruleConfiguration.defaultRank simply to ruleConfiguration.level and ruleConfiguration.rank.
  • Persist the actual runtime rules configuration in a new array of reportingConfigurationOverride objects at invocation.reportingConfigurationOverrides.

POST-BALLOT NOTES

  • @lgolding, 2019-03-18: Since this involves removing the run.resources property and the resources object (which makes sense, since the all the properties of the resources object have moved to reportingDescriptor and there is nothing left in resources), we may as well combine this with Remove current localization mechanism #325, "Remove current localization mechanism".

@ghost ghost changed the title Provide full metadata objects for notifications? Provide full metadata objects for notifications Feb 11, 2019
ghost pushed a commit that referenced this issue Feb 13, 2019
ghost pushed a commit that referenced this issue Feb 13, 2019
ghost pushed a commit that referenced this issue Feb 13, 2019
ghost pushed a commit that referenced this issue Feb 18, 2019
@ghost ghost self-assigned this Feb 18, 2019
@ghost ghost added the enhancement label Feb 18, 2019
@oasis-tcs oasis-tcs deleted a comment from harleenkohli Mar 1, 2019
@ghost ghost added the e-ballot-3 label Mar 18, 2019
ghost pushed a commit that referenced this issue Mar 22, 2019
ghost pushed a commit that referenced this issue Mar 23, 2019
ghost pushed a commit that referenced this issue Mar 23, 2019
@ghost ghost added impact-breaks-consumers impact-breaks-producers change-draft-available design-approved The TC approved the design and I can write the change draft merged Changes merged into provisional draft. tc-34 labels Mar 23, 2019
@ghost
Copy link

ghost commented Apr 6, 2019

Approved in e-ballot-3.

@ghost ghost closed this as completed Apr 6, 2019
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design-approved The TC approved the design and I can write the change draft e-ballot e-ballot-3 enhancement impact-breaks-consumers impact-breaks-producers merged Changes merged into provisional draft. p1 Priority 1 issue to close resolved-fixed tc-34
Projects
None yet
Development

No branches or pull requests

1 participant