add failed subproject paths to ci scan results #425
Conversation
|
Backwards compatibility summary: |
gautambhat
force-pushed
the
gautam/failed-subprojects-in-results-json
branch
from
35c70a1 to
e5d7b41
Compare
gautambhat
requested review from
a team and
bkettle
and removed request for
bkettle
| : sca_error list | ||
| } | ||
|
|
||
| type ci_sca_unresolved_subproject = { |
There was a problem hiding this comment.
There's already an unresolved_subproject type, so wanted to distinguish this from the other one 👀
There was a problem hiding this comment.
Why can't we reuse that?
|
|
||
| ?sca_unresolved_subprojects | ||
| <doc text=" | ||
| since semgrep 1.4x.y. (update this once PR merges and is part of a release) | ||
| This information is sent to /complete in a different field and structure, but we need to send | ||
| it to /results so that it is available when findings are processed (needed to determine issues' state change). | ||
| "> | ||
| : ci_sca_unresolved_subproject list option; |
There was a problem hiding this comment.
It feels a little silly to be sending subproject info here and also in the supply_chain_stats. I wonder if we can consolidate them?
There was a problem hiding this comment.
i.e. can we make this one have all the information that the /complete one has so that we can stop sending to /complete?
There was a problem hiding this comment.
It's worth thinking about what we want to represent at the conceptual level.
I don't have enough context to be confident here, but my guess is that we'd want one return type that represented something like "the outcome of resolution" (with cases for both successful and failed resolution), and, if we still needed it, supply_chain_stats would only have additional measurements/metrics that could not be derived from looking at the resolution result type. (Which might not be necessary at all—at a first glance, all the fields in subproject_stats are logically describing the outcome of running dependency resolution.)
There was a problem hiding this comment.
one return type that represented something like "the outcome of resolution"
This is basically what's in subproject_stats, yeah, I think it's just a bit poorly named (it inherited a name from somewhere else IIRC)
There was a problem hiding this comment.
Yeah I agree it's not optimal, but I was hoping to avoid having to make a swath of changes to the scan completion logic, which also triggers post-completion events that consume the entire stats object (of which supply chain stats is one constituent).
I could move the entirety of ci_scan_complete_stats to /results, but am just a little anxious about unintended or incomplete scan completion behaviour 🤔
There was a problem hiding this comment.
I don't think the supply chain stats are actually used for anything except storing in S3 (and maybe the DB? Can't remember)
There was a problem hiding this comment.
Yeah they're uploaded to S3 with the rest of the stats object (not the DB). I guess stats can be uploaded earlier - I'll play around with this and get back here
squaresurf
left a comment
squaresurf
left a comment
There was a problem hiding this comment.
Great work. I'm curious if it would make sense to simplify the data we plan on sending the semgrep-app.
| ?sca_unresolved_subprojects | ||
| <doc text=" | ||
| since semgrep 1.4x.y. (update this once PR merges and is part of a release) | ||
| This information is sent to /complete in a different field and structure, but we need to send | ||
| it to /results so that it is available when findings are processed (needed to determine issues' state change). | ||
| "> | ||
| : ci_sca_unresolved_subproject list option; |
There was a problem hiding this comment.
question: what do you think about changing this to have a default value of an empty list rather than an option? Your semgrep-app PR would be simplified a little by not having to work with a bunch of values that could be None as opposed to being a list of 0 or more.
This comment and the thread as a whole gives some more context on this idea.
| ?sca_unresolved_subprojects | |
| <doc text=" | |
| since semgrep 1.4x.y. (update this once PR merges and is part of a release) | |
| This information is sent to /complete in a different field and structure, but we need to send | |
| it to /results so that it is available when findings are processed (needed to determine issues' state change). | |
| "> | |
| : ci_sca_unresolved_subproject list option; | |
| ~sca_unresolved_subprojects | |
| <doc text=" | |
| since semgrep 1.4x.y. (update this once PR merges and is part of a release) | |
| This information is sent to /complete in a different field and structure, but we need to send | |
| it to /results so that it is available when findings are processed (needed to determine issues' state change). | |
| "> | |
| : ci_sca_unresolved_subproject list; |
| ?sca_unresolved_subprojects | ||
| <doc text=" | ||
| since semgrep 1.4x.y. (update this once PR merges and is part of a release) | ||
| This information is sent to /complete in a different field and structure, but we need to send | ||
| it to /results so that it is available when findings are processed (needed to determine issues' state change). | ||
| "> | ||
| : ci_sca_unresolved_subproject list option; |
There was a problem hiding this comment.
question: ci_sca_unresolved_subproject contains quite a bit of data. It appears that we are only using a flattened list of filepaths in your semgrep-app PR. Do you foresee us using the other data in the future?
make setup && maketo update the generated code after editing a.atdfile (TODO: have a CI check)For example, the Semgrep backend need to still be able to consume data
generated by Semgrep 1.50.0.
See https://atd.readthedocs.io/en/latest/atdgen-tutorial.html#smooth-protocol-upgrades
Note that the types related to the semgrep-core JSON output or the
semgrep-core RPC do not need to be backward compatible!
semgrep-proprietaryare approved and ready to merge once this PR is merged