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

Integrate snippet scanning into FOSSA CLI #1298

Merged
merged 9 commits into from
Oct 12, 2023
Merged

Conversation

jssblck
Copy link
Member

@jssblck jssblck commented Oct 10, 2023

Overview

Integrates snippet scanning into FOSSA CLI as fossa snippets.

Acceptance criteria

Users no longer need to run this as a separate subcommand.

Testing plan

I tested manually:


fossa-cli on ⑆ millhone/integrate [$!] via λ 9.4.7 via 🦀 v1.72.0 took 8s
❯ cabal run fossa snippets analyze -- ~/projects/scratch/src -o ~/projects/scratch/snippets --overwrite-output
 INFO main{target=/Users/jessica/projects/scratch/src/}: Analyzing local snippet matches
 INFO main{target=/Users/jessica/projects/scratch/src/}: Finished matching 20 snippets out of 3 files to 68 matches


fossa-cli on ⑆ millhone/integrate [$!] via λ 9.4.7 via 🦀 v1.72.0
❯ cabal run fossa snippets commit -- ~/projects/scratch/src --analyze-output ~/projects/scratch/snippets --overwrite-fossa-deps
 INFO main{target=/Users/jessica/projects/scratch/src/}: Committing local snippet matches
 INFO main{target=/Users/jessica/projects/scratch/src/}: Wrote output to '/Users/jessica/projects/scratch/src/fossa-deps.yml' with dependency count: 9


fossa-cli on ⑆ millhone/integrate [$!] via λ 9.4.7 via 🦀 v1.72.0
❯ cat /Users/jessica/projects/scratch/src/fossa-deps.yml
───────┬────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
       │ File: /Users/jessica/projects/scratch/src/fossa-deps.yml
───────┼────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   1   │ referenced-dependencies:
   2   │ - type: git
   3   │   name: github.com/fs-c/maniac
   4   │   version: v0.7.0-beta
   5   │ - type: git
   6   │   name: github.com/shiquan/PISA
   7   │   version: v0.10a
   8   │ - type: git
   9   │   name: github.com/shiquan/PISA
  10   │   version: v0.10b
  11   │ - type: git
  12   │   name: github.com/shiquan/PISA
  13   │   version: v0.11a
  14   │ - type: git
  15   │   name: github.com/shiquan/PISA
  16   │   version: v0.12
  17   │ - type: git
  18   │   name: github.com/shiquan/PISA
  19   │   version: v0.12a
  20   │ - type: git
  21   │   name: github.com/shiquan/PISA
  22   │   version: v0.4-alpha
  23   │ - type: git
  24   │   name: github.com/shiquan/PISA
  25   │   version: v0.6
  26   │ - type: git
  27   │   name: github.com/wooorm/levenshtein.c
  28   │   version: 2.0.2
───────┴────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

Risks

No major risk, although we're taking on some backwards compatibility commitments here.

Metrics

None

References

https://fossa.atlassian.net/browse/ANE-1136

Checklist

  • I added tests for this PR's change (or explained in the PR description why tests don't make sense).
  • If this PR introduced a user-visible change, I added documentation into docs/.
  • If this change is externally visible, I updated Changelog.md. If this PR did not mark a release, I added my changes into an # Unreleased section at the top.
  • If I made changes to .fossa.yml or fossa-deps.{json.yml}, I updated docs/references/files/*.schema.json. You may also need to update these if you have added/removed new dependency type (e.g. pip) or analysis target type (e.g. poetry).
  • If I made changes to a subcommand's options, I updated docs/references/subcommands/<subcommand>.md.

@jssblck jssblck marked this pull request as ready for review October 11, 2023 01:38
@jssblck jssblck requested a review from a team as a code owner October 11, 2023 01:38
@jssblck jssblck requested a review from spatten October 11, 2023 01:38
Copy link
Contributor

@spatten spatten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

I added some minor comments on the docs, and thought we should add a unit test on mkCmd, but none of that blocks approval

docs/references/subcommands/snippets/commit.md Outdated Show resolved Hide resolved
src/App/Fossa/Config/Snippets.hs Show resolved Hide resolved
docs/references/subcommands/snippets/commit.md Outdated Show resolved Hide resolved
root = unBaseDir $ analyzeScanDir conf

mkCmd :: BinaryPaths -> Path Abs Dir -> AnalyzeConfig -> Command
mkCmd bin root AnalyzeConfig{..} =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be worth adding some tests for mkCmd here and for Snippets/Commit.hs too

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO tests for what amounts to argument shuffling don't make a lot of sense (I don't write them for any application, nor for main functions, which is effectively what this is).

That aside, we don't actually have a great way to test this today because we'd need to run an actual analysis inside the test to do so. I've filed this ticket to add the ability to test passed arguments: https://fossa.atlassian.net/browse/ANE-1235

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to go ahead and merge this for now but happy to talk more on this and do a follow up PR if needed!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm fine with merging without this. This is on the edge of needing testing and it's not worth doing if it's hard to do.

I think you could do it, though, by exporting mkCmd and just testing that. I do that for themisFlags, which is pretty much the same thing as mkCmd, here: https://github.com/fossas/fossa-cli/blob/master/test/App/Fossa/RunThemisSpec.hs

@jssblck jssblck merged commit e3c5544 into master Oct 12, 2023
@jssblck jssblck deleted the millhone/integrate branch October 12, 2023 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants