feat: send deploy validations report on secret scan #6205
Conversation
| This pull request adds or modifies JavaScript ( |
aitchiss force-pushed the suzanne/wrfl-2391-persist-secret-scanning-output-on-deploy branch from 49fdc90 to c6ba087 Compare | This pull request adds or modifies JavaScript ( |
aitchiss force-pushed the suzanne/wrfl-2391-persist-secret-scanning-output-on-deploy branch from c6ba087 to cc6d820 Compare | This pull request adds or modifies JavaScript ( |
aitchiss force-pushed the suzanne/wrfl-2391-persist-secret-scanning-output-on-deploy branch from cc6d820 to 9ea5e26 Compare | This pull request adds or modifies JavaScript ( |
| t.truthy(request.body.secrets_scan.secretsScanMatches) | ||
| }) | ||
| | ||
| test('secrets scan does not send report to API if deploy ID is string 0', async (t) => { |
There was a problem hiding this comment.
When is the deploy id '0'?
Could we instead say "secrets scan does not send report to API when ....(situation that causes deploy id to be 0)"
| t.true(requests.length === 0) | ||
| }) | ||
| | ||
| test('secrets scan does not send report to API if deploy ID is undefined', async (t) => { |
There was a problem hiding this comment.
Same note as above here, when is the deploy id not set?
There was a problem hiding this comment.
It actually shouldn't happen, as far as I can tell (we have tests that indicate it should always be set to the default of '0' at least) so I'll remove this spec
| deployId: string | ||
| }) { | ||
| try { | ||
| // @ts-expect-error API type is not defined |
There was a problem hiding this comment.
Ideally we aren't adding in a new @ts-expect-error unless we absolutely need to
There was a problem hiding this comment.
I'll see what I can do. It doesn't look like NetlifyAPI contains the type definitions for the dynamically created functions like createPluginRun or this updateDeployValidations
There was a problem hiding this comment.
I found the correct type for the API methods but unfortunately the updateDeployValidations isn't included. Inspecting the DynamicMethods it seems like createPluginRun which is also used in the codebase also isn't included in the types. Both of these are private/internal-only methods which I am assuming we don't want to bundle into the type definitions, which I think should be why they don't appear.
There was a problem hiding this comment.
Why don't we want to bundle them into the type definitions? I'm not saying we do want to, I just want to understand
There was a problem hiding this comment.
I'd need to dig into it more but I'm assuming it's because it's a public package and we don't actively want external folks using those methods. Same as we don't publish the internal methods in https://open-api.netlify.com/
| This pull request adds or modifies JavaScript ( |
| This pull request adds or modifies JavaScript ( |
| This pull request adds or modifies JavaScript ( |
| This pull request adds or modifies JavaScript ( |
| try { | ||
| // @ts-expect-error Property 'updateDeployValidations' does not exist on type 'DynamicMethods'. This is a private/internal-only method and isn't generated in the type definitions. | ||
| await api.updateDeployValidations({ deploy_id: deployId, body: { secrets_scan: secretScanResult } }) | ||
| } catch (e) { |
There was a problem hiding this comment.
Ideally we are catching specific named errors here, but it's possible we don't know at this time what could go wrong with this call.
There was a problem hiding this comment.
a malformed payload is what springs to mind, but it's still behind a feature flag and being iterated on, so I'd suggest we monitor initially and improve as we go
| }, | ||
| ) | ||
| | ||
| if (deployId && deployId !== '0') { |
There was a problem hiding this comment.
If the deployId cannot be unset, is it still necessary to check the value in this clause?
There was a problem hiding this comment.
Yeah I put it here out of an abundance of caution, but it does look like we always have either a deploy_id or the default one:
build/packages/config/src/options/main.js
Line 47 in 6445846
| This pull request adds or modifies JavaScript ( |
| This pull request adds or modifies JavaScript ( |
aitchiss deleted the suzanne/wrfl-2391-persist-secret-scanning-output-on-deploy branch 
🎉 Thanks for submitting a pull request! 🎉
Summary
Fixes https://linear.app/netlify/issue/WRFL-2391/persist-secret-scanning-output-on-deploy
Send results of secret scan to API on completion.
For us to review and ship your PR efficiently, please perform the following steps:
we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or
something that`s on fire 🔥 (e.g. incident related), you can skip this step.
your code follows our style guide and passes our tests.
A picture of a cute animal (not mandatory, but encouraged)