Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the ability to filter report #79

Merged
merged 16 commits into from Mar 4, 2019

Conversation

Projects
None yet
4 participants
@Raynos
Copy link
Contributor

Raynos commented Feb 27, 2019

This PR implements #63

  • filter by compliance
  • filter by security
  • write tests for functionality

The other half is implemented in #82

@Fishrock123

This comment has been minimized.

Copy link
Contributor

Fishrock123 commented Feb 27, 2019

Some thoughts:

Related to #75 I wonder if either that should be done first or some of the necessary changes should be done here - i.e. more logic should probably be moved out from the printing parts in lib/report/ and call (even if actually done by some util) in the report command before going to the output format.

The actual interface ... I think it would probably be more consistent to do --filter <comma separated terms>, ideally where the comma-separated terms in one --filter are inclusive (and) (and also perhaps additional --filter statements are exclusive (or), but perhaps that is out of scope).

I don't really want to build a query language but I think that --filter might be better long term. It's also a little more descriptive.

@Raynos Raynos force-pushed the filter-compliance branch from 07c2eac to 4042209 Feb 28, 2019

@Raynos Raynos changed the base branch from master to fishrock/tests Feb 28, 2019

@Raynos Raynos marked this pull request as ready for review Feb 28, 2019

@Raynos

This comment has been minimized.

Copy link
Contributor Author

Raynos commented Feb 28, 2019

This PR has gotten big enough ; it's rebased on top of the tests work fishrock did.

I'm going to implement filtering in a seperate (stacked) PR.

@juliangruber
Copy link
Member

juliangruber left a comment

..otherwise LVGTM! Very concise, only changing exactly what needs to be changed, and added tests as well.

@@ -123,6 +123,8 @@ function optionsList () {
{${COLORS.light1} ncm} {${COLORS.yellow} report}
{${COLORS.light1} ncm} {${COLORS.yellow} report} {${COLORS.teal} <directory>}
{${COLORS.teal} -l, --long} {white Expanded output with module list}
{${COLORS.teal} --compliance} {white Expanded output with compliance failures}
{${COLORS.teal} --security} {white Expanded output with security failures}

This comment has been minimized.

@juliangruber

juliangruber Feb 28, 2019

Member

no shorthands?

This comment has been minimized.

@Raynos

Raynos Feb 28, 2019

Author Contributor

ill add them.

Show resolved Hide resolved lib/ncm-style.js
@@ -15,7 +17,10 @@ const {
const L = console.log
const chalk = require('chalk')

function shortReport (report, dir) {
function shortReport (report, dir, argv) {
const filterCompliance = argv ? !!argv.compliance : false

This comment has been minimized.

@juliangruber

juliangruber Feb 28, 2019

Member

are those not boolean by default? also, can be refactored to

Suggested change
const filterCompliance = argv ? !!argv.compliance : false
const filterCompliance = argv && argv.compliance

(this isn't go :P)

This comment has been minimized.

@Raynos

Raynos Feb 28, 2019

Author Contributor

they are only booleans if we tell minimist to parse them as booleans.

I'd rather cast untrusted user input then have weird types later on.

hasComplianceFailure = true
break
}
}

This comment has been minimized.

@juliangruber

juliangruber Feb 28, 2019

Member

can be refactored to

const hasComplianceFailure = pkg.scores.find(score =>
  score.group === 'compliance' && !score.pass)

or even

out.push(...report.filter(pkg =>
  pkg.scores.find(score => score.group === 'compliance' && !score.pass)))

This comment has been minimized.

@juliangruber

juliangruber Feb 28, 2019

Member

I find those functional approaches easier to read than the break early exit

This comment has been minimized.

@Raynos

Raynos Feb 28, 2019

Author Contributor

but look at all those allocated temporary arrays and closure allocations :(

This comment has been minimized.

@juliangruber

juliangruber Feb 28, 2019

Member

thinking about allocations = premature optimization. This is a cli tool :P

This comment has been minimized.

@Fishrock123

Fishrock123 Feb 28, 2019

Contributor

I would rather write something easier to read, but as my review suggests, I think there is probably an overall better way to do this.

failure.severity === 'CRITICAL'
)) {
hasSecurityFailure = true
break

This comment has been minimized.

@juliangruber

juliangruber Feb 28, 2019

Member

same as above

@Fishrock123 Fishrock123 force-pushed the fishrock/tests branch from 22145a5 to 2c54e7f Feb 28, 2019

@Fishrock123
Copy link
Contributor

Fishrock123 left a comment

I don't really want to leave these flags in with this form for launch...

If we can get it in for launch I think we should table it for now and come back doing it properly in a follow-up release.

The kind of logic I was thinking of internally would be something like:

const knownGroupTerms = [ 'security', 'compliance' ]
const groupTerms = []
for (const term of filter.toLowerCase().split(',')) {
  if (knownGroupTerms.includes(term)) groupTerms.push(term)
}

if (groupTerms || severityTerms) {
  for (const score of pkg.scores) {
    if (score.pass) continue
    if (groupTerms && !groupTerms.includes(score.group)) continue
    if (severityTerms && !severityTerms.includes(score.severity) continue
    filtered.push(<whatever we need to add to this array>)
  }
}

I think something like that should work (untested). Lmk if you see something where that wouldn't work well though.

@@ -1,3 +1,3 @@
/node_modules/

test/fixtures/*/node_modules

This comment has been minimized.

@Fishrock123

Fishrock123 Feb 28, 2019

Contributor

The tests won't work with this on a fresh install...

I intentionally put placeholders there to not have to do actual installs. Maybe we can find a smaller module that is has failures?

This comment has been minimized.

@Raynos

Raynos Feb 28, 2019

Author Contributor

i verified that the package-lock.json is sufficient for the tests to work; node_modules should be optional.

Show resolved Hide resolved lib/ncm-style.js
@Raynos

This comment has been minimized.

Copy link
Contributor Author

Raynos commented Feb 28, 2019

@Fishrock123

I'm implementing the filter version of the CLI arg in another PR ( #82 ).

I left --compliance as an alias for --filter=compliance and likewise with --security as an alias for --filter=security

@Raynos

This comment has been minimized.

Copy link
Contributor Author

Raynos commented Feb 28, 2019

i'll go back to the implementation once it's working & tested and see if i can collapse all the loops into a single loop.

@Raynos Raynos force-pushed the filter-compliance branch from 4042209 to 969cdfa Mar 1, 2019

@Raynos

This comment has been minimized.

Copy link
Contributor Author

Raynos commented Mar 1, 2019

Addressed feedback.

@Raynos

This comment has been minimized.

Copy link
Contributor Author

Raynos commented Mar 1, 2019

@Fishrock123 in #82 i made the loops simpler but unfortunately its not as elegant because we have pkg.scores and pkg.failures

@Fishrock123

This comment has been minimized.

Copy link
Contributor

Fishrock123 commented Mar 1, 2019

we have pkg.scores and pkg.failures

failures is a sort-of duplicate of scores. We may want to remove that from the code entirely eventually and just always use scores and check score.pass when we need to.

@Fishrock123
Copy link
Contributor

Fishrock123 left a comment

Aight, good enough for me.

Can you re-target this to master and also please squash a bit? So long as the commits each pass changes on their own would be good.

@Fishrock123 Fishrock123 changed the base branch from fishrock/tests to master Mar 1, 2019

@juliangruber
Copy link
Member

juliangruber left a comment

except for those minor things this looks very good to me! 👍

function shortReport (report, dir) {
function shortReport (report, dir, argv) {
const filterCompliance = argv ? !!(argv.compliance || argv.c) : false
const filterSecurity = argv ? !!(argv.security || argv.s) : false

This comment has been minimized.

@juliangruber

juliangruber Mar 3, 2019

Member

could we update our stdarg parsing to alias argv.s to argv.security etc so we don't have to check both flags?


if (options.filterCompliance) {
for (const pkg of report) {
let hasComplianceFailure = pkg.scores.some(

This comment has been minimized.

@juliangruber

juliangruber Mar 3, 2019

Member
Suggested change
let hasComplianceFailure = pkg.scores.some(
const hasComplianceFailure = pkg.scores.some(
if (!pkg.failures) {
continue
}
let hasSecurityFailure = pkg.failures.some(

This comment has been minimized.

@juliangruber

juliangruber Mar 3, 2019

Member
Suggested change
let hasSecurityFailure = pkg.failures.some(
const hasSecurityFailure = pkg.failures.some(

@Raynos Raynos force-pushed the filter-compliance branch from 969cdfa to a226fb8 Mar 4, 2019

@Raynos Raynos merged commit 8316601 into master Mar 4, 2019

1 check passed

CodeBuild Build was a success.
Details

@Raynos Raynos deleted the filter-compliance branch Mar 4, 2019

@Jazneil Jazneil added this to the Sprint 10 (2/26 - 3/18) milestone Mar 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.