Fix: Propagate --insecure parameter #78
Conversation
| ignoreUnknownCA: boolean; | ||
| } | ||
| declare const global: Global; | ||
|
|
||
| const agentOptions = { | ||
| keepAlive: true, | ||
| maxSockets: 100, // Maximum number of sockets to allow per host. Defaults to Infinity. | ||
| maxFreeSockets: 10, | ||
| freeSocketTimeout: 60000, // // Maximum number of sockets to leave open for 60 seconds in a free state. Only relevant if keepAlive is set to true. Defaults to 256. |
There was a problem hiding this comment.
@Arvi3d I am fairly sure this settings does nothing here 😇 It is not supported at https://nodejs.org/docs/latest-v12.x/api/http.html#http_new_agent_options and it looks like it is used only, when https://www.npmjs.com/package/agentkeepalive is around. I'd vote for removing it and typing agentOptions properly.
miiila
force-pushed
the
fix/propagate-insecure
branch
2 times, most recently
from
da83d13 to
37eaa19
Compare
| @@ -3,12 +3,19 @@ import http from 'http'; | |||
| import https from 'https'; | |||
| import emitter from './emitter'; | |||
|
|
|||
| // For a compatibility with Snyk CLI | |||
| export declare interface Global extends NodeJS.Global { | |||
| ignoreUnknownCA: boolean; | |||
There was a problem hiding this comment.
Required: Does CA stand for certification Authorities? Could we add a comment explaining that?
There was a problem hiding this comment.
CA should be widely known abbrev, so I don't wanna to add a comment just because of that. However, I can elaborate on a comment explaining, how it can be turned on.
| @@ -3,12 +3,19 @@ import http from 'http'; | |||
| import https from 'https'; | |||
| import emitter from './emitter'; | |||
|
|
|||
| // For a compatibility with Snyk CLI | |||
| export declare interface Global extends NodeJS.Global { | |||
There was a problem hiding this comment.
Question: maybe I am lacking some understanding but why do you have to extend the global object? Is this because typescript does not define the property that is already there?
There was a problem hiding this comment.
Global is an Interface with defined properties and TypeScript interfaces are closed for additions. So we need to create our own interface.
| @@ -42,7 +42,7 @@ describe('Requests to public API', () => { | |||
| if (response.type === 'error') return; | |||
| expect(new Set(response.value.configFiles)).toEqual(new Set(['.dcignore', '.gitignore'])); | |||
| expect(new Set(response.value.extensions)).toEqual( | |||
| new Set(['.es', '.es6', '.htm', '.html', '.js', '.jsx', '.py', '.ts', '.tsx', '.vue', '.java']), | |||
| new Set(['.es', '.es6', '.htm', '.html', '.js', '.jsx', '.py', '.ts', '.tsx', '.vue', '.java', '.java-dummy']), | |||
There was a problem hiding this comment.
Question: Why do we now return .java-dummy in this list?
There was a problem hiding this comment.
Ask t-42 ¯_(ツ)_/¯ Looks like something reachability related.
This is important for a unified behaviour when running both snyk test and snyk code test
miiila
force-pushed
the
fix/propagate-insecure
branch
from
37eaa19 to
0290866
Compare
|
🎉 This PR is included in version 3.5.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
To unify behaviour with default
snyk testcommand, this PR takes an advantage of snyk/cli@5ae3182#diff-1bc78dd2ada161f97e1a1e595957fbe2487468b6a81191600acf0aa63769ad88R136