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

Ane 1028 broker fix fatal exit #130

Merged
merged 16 commits into from
Sep 18, 2023
Merged

Conversation

JeffreyHuynh1
Copy link
Contributor

@JeffreyHuynh1 JeffreyHuynh1 commented Sep 7, 2023

Overview

Broker fails fatally if FOSSA CLI doesn't find targets (Fixed in previous PR). We want Broker Fix to surface these errors.

Acceptance criteria

Broker does not fatally exit when scanning a project that does not have analysis targets.

Broker reports such errors during broker fix.

Note: this doesn’t mean “reports historical errors”; I mean “during broker fix Broker should download the integration and run fossa analyze -o on it, and report an error if that fails”.

Testing plan

  1. Run Broker Fix on an empty repo. This should display the suggestions on how to debug the error (Broker doesn't fatally exit)
  2. Unit Test: with_failing_http_no_auth_integration_scan() [tests/fix.rs]
    2.a Tests a failed scan
  3. Unit Test: with_failing_http_no_auth_download_cli() [tests/fix.rs]
    3.a Tests failure on downloading fossa cli during attempt to scan integration

Risks

I created this function that propagates potential errors during scan.

check_integration_scan()

I created new variants of all the possible errors that could be encountered but didn't provide a 'fix' for all of them. Only for errors :
Error::DownloadFossaCli & Error::CheckIntegrationScan

I was unsure of the fixes for these errors:
Error::EmptyReferences & Error::CloneReference

Please let me know if this if is fine for now or if you want me to implement the fixes for these errors.

Would there ever be a case where a user would have empty references?

References

Checklist

  • I added tests for this PR's change (or explained in the PR description why tests don't make sense).

@JeffreyHuynh1 JeffreyHuynh1 requested a review from a team as a code owner September 7, 2023 00:36
src/cmd/fix.rs Outdated Show resolved Hide resolved
src/cmd/fix.rs Outdated Show resolved Hide resolved
@@ -10,7 +10,7 @@ debugging:
integrations:
- type: git
poll_interval: 1h
remote: https://github.com/fossas/broker.git
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the remote because the integration test: with_successful_http_no_auth_integration() in fix.rs wasn't working as intended.

I talked to scott and he told me that it is because broker removes PATH field when using fossa analyze through broker. The results from fossa analyze and broker would differ when running a scan on the repo: https://github.com/fossas/broker.git due to static analysis. Therefore I changed the remote that we are scanning.

.github/workflows/dynamic-analysis.yml Outdated Show resolved Hide resolved
src/api/remote.rs Outdated Show resolved Hide resolved
src/api/remote/git.rs Outdated Show resolved Hide resolved
src/cmd/fix.rs Outdated Show resolved Hide resolved
src/cmd/fix.rs Outdated Show resolved Hide resolved
tests/it/fix.rs Outdated Show resolved Hide resolved
tests/it/fix.rs Outdated Show resolved Hide resolved
@JeffreyHuynh1 JeffreyHuynh1 enabled auto-merge (squash) September 13, 2023 00:01
@JeffreyHuynh1 JeffreyHuynh1 enabled auto-merge (squash) September 13, 2023 00:03
Copy link
Member

@jssblck jssblck left a comment

Choose a reason for hiding this comment

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

lgtm after the windows comment!

Comment on lines +107 to +110
#[cfg(target_family = "windows")]
fn cli_command() -> &'static str {
"PATH ; fossa.exe analyze -o"
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is how env vars are set for the current session in powershell. Please validate- if I'm wrong I'm happy to accept, but otherwise please fix it.

Also, I think I never mentioned before that when we say "windows" we mean "windows in powershell". Sorry about that.

src/cmd/run.rs Show resolved Hide resolved
@JeffreyHuynh1 JeffreyHuynh1 merged commit ced9628 into main Sep 18, 2023
12 checks passed
@JeffreyHuynh1 JeffreyHuynh1 deleted the ANE-1028-Broker-Fix-Fatal-Exit branch September 18, 2023 19:39
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