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

SARIF_LEVEL_OVERRIDE, SARIF_KIND_OVERRIDE #2229

Closed
michaelcfanning opened this issue Jan 9, 2021 · 0 comments · Fixed by #2273
Closed

SARIF_LEVEL_OVERRIDE, SARIF_KIND_OVERRIDE #2229

michaelcfanning opened this issue Jan 9, 2021 · 0 comments · Fixed by #2273
Assignees

Comments

@michaelcfanning
Copy link
Member

When we complete #2228, we should add support for environment variables that completely replace anything on the command-line if they are encountered. So,

set SARIF_LEVEL_OVERRIDE=Error;Warning;Note

could be used to automatically opt into 'experimental' failure messages (which has the 'note' level set on them and would otherwise be filtered). this is useful to look at the behavior of existing analysis systems without requiring that tools be reconfigured.

Notes:

  • We should think about a name convention that maps to arg names. I've chosed SARIF_[ARGNAME]_OVERRIDE. We could use this same standard for any/all arguments we decide can be overridden.
  • It is pretty dangerous to conditionally drive tool behavior based on environment variables. One protection against this is to require some explicit command-line argument that allows for these variables to be read/override arguments. That requires users to update all their command-lines to enable the functionality before it works.
  • Requiring opt-in to this override policy could be burdensome. If we do not require it, we should definitely emit an extremely prominent notification which can't be disabled via all our magical mechanisms that reports that an argument is being overridden. Error notifications in particular can never be disabled. If we raise an error notification for this, however, the experiment might break systems that halt on errors, preventing users from experimenting with lighting up non-default analysis. So as usual, there's more nuance here than one might think.
eddynaka added a commit that referenced this issue Jan 29, 2021
…2273)

* Move analyzeTestOptions to same project as al other options.  Add EnvironmentVariableGetter and OptionsInterpretter classes

* Nullchecking, formatting

* Add unit test, minor refactoring

* Formatting

* More completely implement pattern, correct unit tests, remove transform option

* Add "if debug" statements

* Add comment to releasehistory

Co-authored-by: Eddy Nakamura <eddynaka@gmail.com>
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 a pull request may close this issue.

2 participants