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

Strict mode #1463

Merged
merged 11 commits into from
Sep 13, 2024
Merged

Strict mode #1463

merged 11 commits into from
Sep 13, 2024

Conversation

JeffreyHuynh1
Copy link
Contributor

@JeffreyHuynh1 JeffreyHuynh1 commented Aug 30, 2024

Overview

We should allow users to enforce that the first strategy within our list of strategies passes. This is to ensure ‘ideal’ results and that no fallback strategies are ran.

Acceptance criteria

  • When running fossa analyze --strict :

    • Turn warnings into fatalities

    • Ensure that the first strategy in the list passes

Testing plan

Manually testing:

  • fossa analyze --strict --debug (I scanned a Maven project)
  • In the plugin strategy , emit a fatal error e.g. fatal.MissingDeepDeps
  • See that we do not fallback to the other strategies (deptree, pom, etc) and analysis halts

Risks

Would like to add some automated tests but wanted to get some opinions. Would it be sufficient to just create a test for guardStrictMode bc that's the main logic that powers strict mode? Unsure on how to best create tests for strict mode for specific languages / package managers.

Metrics

References

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 PR added docs, I added links as appropriate to the user manual's ToC in docs/README.ms and gave consideration to how discoverable or not my documentation is.
  • 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 AND I have updated example files used by fossa init command. 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.

@JeffreyHuynh1 JeffreyHuynh1 marked this pull request as ready for review August 30, 2024 22:42
@JeffreyHuynh1 JeffreyHuynh1 requested a review from a team as a code owner August 30, 2024 22:42
Copy link
Contributor

@csasarak csasarak left a comment

Choose a reason for hiding this comment

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

This looks pretty good! I think the main changes I'd ask for here are some clarity around how --static-only-analysis and --strict interact and a few comments.

docs/references/subcommands/analyze.md Outdated Show resolved Hide resolved
src/App/Fossa/Analyze.hs Outdated Show resolved Hide resolved
integration-test/Analysis/FixtureUtils.hs Outdated Show resolved Hide resolved
src/App/Fossa/Container/Sources/DockerArchive.hs Outdated Show resolved Hide resolved
src/App/Types.hs Show resolved Hide resolved
src/Strategy/Cocoapods.hs Outdated Show resolved Hide resolved
src/Strategy/Bundler.hs Outdated Show resolved Hide resolved
src/Strategy/Pub.hs Outdated Show resolved Hide resolved
docs/references/subcommands/analyze.md Outdated Show resolved Hide resolved
@csasarak
Copy link
Contributor

csasarak commented Sep 3, 2024

One thing I forgot to mention is that I think this should be pretty easy to write tests for. I think that if you go to the tests for each of these strategies there is likely one that gets results from the "ideal" analysis. You can run the analysis using strict mode and just check that the output equals the output of the other test. I'd like it if you could try to make a few of these and see what the effort is like. Let me know if you'd like any help.

@carloskcheung
Copy link
Contributor

carloskcheung commented Sep 3, 2024 via email

Copy link
Contributor

@csasarak csasarak 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. Please address my comments, but I don't think anything there should need me to look this over again unless you want me to.

docs/references/subcommands/analyze.md Outdated Show resolved Hide resolved
docs/references/subcommands/analyze.md Outdated Show resolved Hide resolved
@JeffreyHuynh1 JeffreyHuynh1 merged commit 3a00319 into master Sep 13, 2024
19 checks passed
@JeffreyHuynh1 JeffreyHuynh1 deleted the strict-mode branch September 13, 2024 20:20
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.

3 participants