Skip to content
This repository has been archived by the owner on Apr 1, 2022. It is now read-only.

Run test as monorepo OR normal, not both #321

Merged
merged 6 commits into from
Aug 23, 2021
Merged

Run test as monorepo OR normal, not both #321

merged 6 commits into from
Aug 23, 2021

Conversation

skilly-lily
Copy link
Contributor

@skilly-lily skilly-lily commented Aug 6, 2021

Overview

Previously, we tried running both normal build polling and monorepo polling at the same time. However this causes us to report incorrect errors during normal build failures. This PR attempts to detect the correct project type, and fall back to normal builds unless the user tells us to fall back to normal builds.

Requires fossas/FOSSA#6311, which has been deployed in cloud in version 3.34.19

Acceptance criteria

  • fossa test and fossa report use the same logic to detect project types.
  • We don't hide or obscure errors in build polling.

Testing plan

  • Run analyze/test/report for a normal project
  • Run test and report on a monorepo project, against prod.
    • This will not work on prod until the referenced PR is deployed.
  • Run test and report on a monorepo project, against a core version without the /api/cli/:locator/project route.
    • This will work on prod UNTIL the mentioned PR is deployed.

Risks

I'm not sure this is the right move. We are potentially introducing a failure mode where on-prem customers now have to enable a certain flag to run correctly, where they didn't before. However, I don't see a better way to handle this without obscuring errors.

References

Related to fossas/team-analysis#708

Checklist

  • I added tests for this PR's change (or confirmed tests are not viable).
  • If this PR introduced a user-visible change, I added documentation into docs/.
  • I updated Changelog.md if this change is externally facing. If this PR did not mark a release, I added my changes into an # Unreleased section at the top.
  • I linked this PR to any referenced GitHub issues, if they exist.

@cnr
Copy link
Contributor

cnr commented Aug 6, 2021

Rather than introducing a new flag, falling back to the old behavior would be nice here. It'll be painful to remove that flag later

@skilly-lily
Copy link
Contributor Author

I figured we wouldn't remove it, just deprecate it and make it a no-op

@jssblck
Copy link
Member

jssblck commented Aug 6, 2021

Rather than introducing a new flag, falling back to the old behavior would be nice here. It'll be painful to remove that flag later

@thedeerchild, @enricozb - I think we can ensure our monorepo users are on the latest this specific FOSSA version as soon as it's ready so that we don't have to worry about encountering this fallback. Do you agree, or do you think we need this flag?

@thedeerchild
Copy link

Disagree with the note regarding upgrade cadence, but I think there are few enough that we can manually upgrade them past it. Let's just confirm the Core version somewhere so that we can make sure to upgrade Moto this week (and tell anyone else starting with Monorepo that they need to be above that version).

@skilly-lily
Copy link
Contributor Author

@thedeerchild Confirming: We're removing the --monorepo flag and don't cover the case of out-of-date FOSSA instances here? If you think that we don't need to cover that case this way I'd prefer to not add the flag.

@thedeerchild
Copy link

Correct. Cc/ @enricozb

@skilly-lily skilly-lily removed the request for review from elldritch August 9, 2021 17:28
@skilly-lily
Copy link
Contributor Author

Required FOSSA version is 3.34.19

@skilly-lily skilly-lily requested a review from cnr August 10, 2021 16:54
Copy link
Contributor

@cnr cnr left a comment

Choose a reason for hiding this comment

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

LGTM; left a note suggesting to remove the sections added to the userguide

docs/userguide.md Outdated Show resolved Hide resolved
@skilly-lily skilly-lily merged commit 927d96e into master Aug 23, 2021
@skilly-lily skilly-lily deleted the test-monorepo branch August 23, 2021 23:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants