Support requireReason #5
Conversation
|
@naugtur this is a suuuuuuuuper early something, just an hour of understanding the projects and doing something basic, and I haven't plugged it yet in npm-audit-resolver. But feedback welcome already 🙂 |
auditFile/fileHandle.js
Outdated
| if (!versions[version]) { | ||
| throw Error(`Unrecognized ${FILE.FILENAME} content version ${version}`) | ||
| } | ||
| if (data.version > 0 && data.requireReason && data.requireReason !== "false") { |
There was a problem hiding this comment.
I'd like to avoid adding top-level keys to the schema.
Current schema includes a "rules" field where requireReason could exist.
I'd also prefer to do === true here to avoid ambiguity.
You could add requireReasonMatch that'd contain a regex
There was a problem hiding this comment.
I'd like to avoid adding top-level keys to the schema. Current schema includes a "rules" field where requireReason could exist.
@naugtur ah yes, I misread your comment naugtur/npm-audit-resolver#23 (comment) where you indeed suggest rules.requireReason . 👍, will adjust.
I'd also prefer to do
=== truehere to avoid ambiguity. You could add requireReasonMatch that'd contain a regex
👍 to === true + requireReasonMatch. My intent for a bit of leniency here was to not confuse a user hopping between true and "some regex" then hopping back to true but forgetting to remove the quotes, getting to "true". But this problem happily disappears with === true + requireReasonMatch 🙂.
Further speccing about that:
- should presence of
requireReasonMatchbe enough to force requiring a reason matching a regex? ... - ... or MUST
requireReasonbe ALSO present forrequireReasonMatchto work?
Option 1 might minimize some user surprise, but option 2. feels stricter. Waddayathink?
Also, a general comment: thinking about this since yesterday, I'm not sure using the Djv schema is the right place for this kind of validation. I'll see when I plug it to the CLI, but I feel I won't be able to get satisfying error reporting to the user, e.g. pointing at the precise problematic key.
Feels like this is going to error unprecisely, lacking user guidance on which key to fix, how. Feels like I should keep the tests, but move this logic out of Djv validation and into custom logic. Waddayathink?
There was a problem hiding this comment.
Feels like I should keep the tests, but move this logic out of Djv validation and into custom logic. Waddayathink?
@naugtur I went ahead in this direction, and here's a first minimal viable version (this PR, plus CLI handling in #5 ). How does that look to you? If you are okay with the approach, adding tests and pinging you when ready.
requirereason-poc.mp4
There was a problem hiding this comment.
@naugtur also, one more discussion point: are you okay if I implement an env. var. equivalent to requireReason/requireReasonMatch ?
I understand your point to avoid collision with npm flags, so extra command -line flags are out of question, but for my needs at $work I'd like to requireReason from outside the audit-resolve itself, for the reason that we commonly nuke the file when ignores become unnecessary (e.g. because patched and obtained with a pkglock regen or an npm audit fix), so I can easily see potential for someone to nuke the whole file and not just the decisions, inadvertently scrapping the rules too 😕.
Whereas if I'm able to call NPM_AUDIT_RESOLVER_REQUIRE_REASON=true check-audit in my CI job that currently is just check-audit, the above gotcha cannot happen. Also, having the option out of the resolve file lets me share it in the shared CI task running for all the projects, whereas in the file the config must be duplicated in all the projects 😕. TL;DR: configuration is nice when shareable, and doesn't belong in a mutable non-config within reach of users mistakes.
- → Okay to add env. vars support?
- ... or given the gotcha (in which others will probably fall), would you go all the way and only support the env. var?
There was a problem hiding this comment.
should presence of requireReasonMatch be enough to force requiring a reason matching a regex?
Either this or throw an error if requireReason is missing while requireReasonMatch exists. But requireReasonMatch being sufficient is my favorite option
I liked it as a djv validation. Sorry I couldn't respond in time. Implementation outside is fine too if it maintains the following:
As I said here: https://github.com/naugtur/npm-audit-resolver/pull/61/files#r911390272
I think I want a failure to result from attempting to load a file with missing reasons instead of adding more RESOLUTION types.
RESOLUTION is something that could one day be standardized to some extent and work beyond this tool. (wishful thinking probably, but I did try) and I want to keep it generic.
It's not impossible to convince me otherwise if you have valid reasons.
As for implementing rules as env - We need a separate PR for this to design what they'd look like - I'd hope for them to be somewhat consistent with the rules field in the file.
How about an env var taht lets you merge/override the values in rules? We'd keep the names as an yet unspecified list and letter case consistency (since override would be provided as a json string)
There was a problem hiding this comment.
should presence of requireReasonMatch be enough to force requiring a reason matching a regex?
Either this or throw an error if requireReason is missing while requireReasonMatch exists. But requireReasonMatch being sufficient is my favorite option
Okay, will see what makes more sense as I'm finishing implem & tests.
I liked it as a djv validation.
@naugtur I see no problem coming back to this, but I don't see how I would be able to reach the same CLI-user error quality with it (pointing at the precise entry failing validation). A failed Djv validation results in a global error that says the file failed to validate, but then I'm toast to say something more precise than "validation failed, fix what's wrong and I can't tell you what's wrong" to the user. Or am I missing something? Can you point me to something to take inspiration from?
Implementation outside is fine too if it maintains [what] I said here: I think I want a failure to result from attempting to load a file with missing reasons instead of adding more RESOLUTION types. RESOLUTION is something that could one day be standardized to some extent and work beyond this tool. (wishful thinking probably, but I did try) and I want to keep it generic. It's not impossible to convince me otherwise if you have valid reasons.
I see your point and agree that these new RESOLUTIONs are unlike the other ones, and should be separated into another concern. So, if proceeding with this approach (not Djv validation), will move away from resolutions. That should be doable with a bit of extra argument passing and an extra getter.
As for implementing rules as env - We need a separate PR for this to design what they'd look like - I'd hope for them to be somewhat consistent with the rules field in the file. How about an env var that lets you merge/override the values in rules? We'd keep the names as an yet unspecified list and letter case consistency (since override would be provided as a json string)
A json string override seems legit. However, I'm not sure what you mean by "We'd keep the names as an yet unspecified list and letter case consistency" , can you precise / reword?
Also, should we have:
- A merge (
{a=1, b=2, c=3} + {a=10, b=10} = {a=10, b=10, c=3}) ? - Or a full override (
{a=1, b=2, c=3} + {a=10, b=10} = {a=10, b=10}) ?
There was a problem hiding this comment.
Yeah, I was concerned djv would not give you enough details. I think if you dig deep in that error you get it has some metadata about where in the JSON the error happened, but it won't be enough to say why exactly it failed. I did use custom validations in a different project and got the error message to "barely enough" level.
Since we're agreed on not adding new RESOLUTIONs I'm now happy with both versions djv or not.
I'd go with merge not override.
There was a problem hiding this comment.
Since we're agreed on not adding new RESOLUTIONs I'm now happy with both versions djv or not.
@naugtur implemented without djv and pushed (no tests yet). How does that look?
I'd go with merge not override.
👍 but not implemented yet
|
I like where this is going. Looking forward to what you do next. |
Depends on naugtur/audit-resolve-core#5 . See naugtur#23 .
…dation into existing resolutions code
|
I'm getting ready to release v3 and this will be the next thing to do for v3.1 |
@naugtur wheeeeeeeeee. I haven't touched this (and sister PR) for a long time. As far as I remember, I was waiting for feedback on the "How does that look?" at #5 (comment) . Can you take a look? Also, anything in this PR + sister PR that you look at now and would like reworked? |
|
TBH I want to use the time sitting locked up with COVID gives me to release v3 finally. So I'll come back to this once released. Feel free to try out the nom-audit-resolver@next to see if there's bugs 😉 |
@naugtur godspeed going through your 'vid! And godspeed to v3!
👍. Have been rolling |
2 participants
Required by naugtur/npm-audit-resolver#61 .
See naugtur/npm-audit-resolver#23 .