feat(sms): add scm_run_id to scan-create interface #306
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Backwards compatibility summary: |
|
We're discussing more context on this offline. Will come back to review once things are clearer, thanks! |
aryx
approved these changes
Nov 1, 2024
There was a problem hiding this comment.
Can you add a comment explaining what SCM mean, what are example of values to make things more concrete (it is like "git" or "mercurial" thing?) and vaguely why we need it to better understand the field.
Also 1.95.0 was out yesterday so you probably want 1.96.0
amchiclet
approved these changes
Nov 1, 2024
There was a problem hiding this comment.
Thanks!
Optional: in my mind, I think adding just a bit more context in the PR description through a concrete github example helped me understand this issue.
For SMS, we would like to be able to connect a particular Scan object to the SCM (source control manager) run_id that it is associated with, e.g. so that we can tell GitHub (the SCM) that a PR check (which runs a scan and has a Scan object) is running or completed.
2 tasks
chmccreery
added a commit
that referenced
this pull request
Nov 5, 2024
#308) Update/amendment to #306. Rather than pushing the SCM run id directly through Scan, we want to take a broader approach that more generally unites an SMS scan with all of its surrounding infrastructure. Therefore, we should actually use the managed_scan_id from semgrep-app rather than the id from the SCM, and let the app take care of connecting the managed_scan_id with the scm_run_id. This is not backwards compatible, but since this field was not yet in-use, I think it makes sense to make the change anyway. - [x] I ran `make setup && make` to update the generated code after editing a `.atd` file (TODO: have a CI check) - [x] I made sure we're still backward compatible with old versions of the CLI. For example, the Semgrep backend need to still be able to *consume* data generated by Semgrep 1.17.0. See https://atd.readthedocs.io/en/latest/atdgen-tutorial.html#smooth-protocol-upgrades
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For SMS, we would like to be able to connect a particular Scan object to the SCM (source control manager) run_id that it is associated with. Details here.
If we create the SCM run before the scan exists (desirable because there are actually many steps between us receiving a webhook and a Scan object getting created, leading to a significant time delay), then when we create the Scan, we need to know what the SCM run id is in order to connect it to the Scan and update it as the Scan progresses. This interface change allows us to pass the SCM run from the CLI to the backend.
Changes in
semgrepto follow this PR will actually set this value based on an environment variable and then send it to the app.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.17.0.
See https://atd.readthedocs.io/en/latest/atdgen-tutorial.html#smooth-protocol-upgrades