Add manifest document schema for Snyk configuration#81
Conversation
Test Results240 tests 238 ✅ 50s ⏱️ Results for commit 649b78d. ♻️ This comment has been updated with latest results. |
bschwedler
left a comment
bschwedler
left a comment
There was a problem hiding this comment.
I like the direction of this!
Please feel free to merge this after moving the default values into the template file.
| # wait = 0 # Defaults to 0 if not specified. | ||
| # command = "sleep infinity" # Defaults to sleep infinity to run if not specified. | ||
|
|
||
| # # (Optional) define alternative settings for Snyk scans for this manifest. |
There was a problem hiding this comment.
Sorry for making you refactor this into the template file!
There was a problem hiding this comment.
All good! Should be an easy fix.
| REGEX_SNYK_MONITOR_PROJECT_ENVIRONMENT = re.compile(r"^(frontend|backend|internal|external|mobile|saas|onprem|hosted|distributed)$") | ||
| REGEX_SNYK_MONITOR_PROJECT_LIFECYCLE = re.compile(r"^(development|sandbox|production)$") | ||
| REGEX_SNYK_MONITOR_PROJECT_BUSINESS_CRITICALITY = re.compile(r"^(low|medium|high|critical)$") |
There was a problem hiding this comment.
Would it make sense to define these as a tuple so they are an immutable collection?
Then we could check with val in SNYK_MONITOR_PROJECT_ENVIRONMENTS, or similar.
There was a problem hiding this comment.
I went for the Enum route instead, but similar idea.
| if isinstance(value, str): | ||
| # If value is a string containing commas, parse and validate it as a list of string input values | ||
| if "," in value: | ||
| split_values = value.split(",") | ||
| return_values = [] | ||
| for split_value in split_values: | ||
| if not regex_pat.match(split_value): | ||
| log.warning(message % split_value) | ||
| else: | ||
| return_values.append(split_value) | ||
| if len(return_values) > 0: | ||
| return return_values | ||
| # If value is a single string, validate it as a single string input value | ||
| else: | ||
| if regex_pat.match(value): | ||
| return value | ||
| else: | ||
| log.warning(message % value) | ||
| # If value is a list of strings, validate each string in the list and return the list of valid strings | ||
| if isinstance(value, list): | ||
| return_values = [] | ||
| for val in value: | ||
| if not regex_pat.match(val): | ||
| log.warning(message % val) | ||
| else: | ||
| return_values.append(val) | ||
| if len(return_values) > 0: | ||
| return return_values |
There was a problem hiding this comment.
I think we could probably shorten this in one of two ways:
- coerce the string into a list first, and then do all the validation in the same logic block
- Pull out the common validation logic into a function that we call in both code paths.
There was a problem hiding this comment.
I decided to split it into clean (takes list or string and converts it to a list of strings) and validate_list (validates individual items against some check, warns on invalid, and returns valid). I think the cognitive complexity was significantly reduced by doing that.
| json_file: bool = False | ||
| sarif_file: bool = False | ||
|
|
||
| @field_validator("format", mode="wrap") |
There was a problem hiding this comment.
This intrigues me!
I don't quite understand what the handler object is, but I like how it just falls back to the default, while still allowing the warnings to be logged.
There was a problem hiding this comment.
handler is actually the pydantic validator that would be applied to the field. The wrap mode lets us get in between the validator and validation errors to instead propagate these warnings. There may be a better way to do this, but this was the best I was able to come up with at the time of writing.
|
|
||
| class ManifestSnykTest(BaseModel): | ||
| severity_threshold: Annotated[ | ||
| str, Field(pattern=REGEX_SNYK_TEST_SEVERITY_THRESHOLD) |
There was a problem hiding this comment.
Ooohhh... This makes more sense if pydantic natively supports regex validation instead of having to write our own extra logic to check whether it is in a tuple, as I added in a previous comment.
| log.warning( | ||
| f"Invalid value for snyk.test.output.format, expected '{value}' to match regex pattern " | ||
| f"'{REGEX_SNYK_TEST_OUTPUT_FORMAT.pattern}'. Using default value " | ||
| f"'{DEFAULT_SNYK_TEST_OUTPUT_FORMAT}'." | ||
| ) |
There was a problem hiding this comment.
I like how clear this warning message is.
I wonder if we could do something similar with the default pydantic validators:
Catch the validation errors, write them out to ERROR logs with a more understandable format, and then exit the cli.
There was a problem hiding this comment.
Ay we did pretty much exactly that in #101
| if not REGEX_SNYK_MONITOR_TAG_VALUE.match(value): | ||
| log.warning( | ||
| f"snyk.monitor.tags.{key} value '{value}' does not match regex pattern. Values must be " | ||
| f"alphanumeric and may use '-', '_', ':', '?', '@', '&', '+', '=', '~', and '%'." |
There was a problem hiding this comment.
💜 Much easier to understand than attempting to grok the regex.
There was a problem hiding this comment.
This one it certainly felt necessary to write out haha
Add new Snyk model tests
…and business criticality
ianpittwood
force-pushed
the
snyk-document-schema
branch
from
5b8ba23 to
ddf831c
Compare
bschwedler
left a comment
bschwedler
left a comment
There was a problem hiding this comment.
Thanks for doing this one!
2 participants
Partial implementation of #33
snyksection to Manifest schema for Snyk configuration settingssnyk containersubcommand:test,monitor,sbomsnyk.test.severity_thresholdorsnyk.monitor.environment) is validated via regex patternsManifestSnykclasses