[WOR-849][WOR-850][WOR-852] Create BPM billing profile for GCP billing projects. #2698
Conversation
|
jenkins retest |
cahrens
changed the title
cahrens
requested review from
a team,
blakery,
okotsopoulos and
marctalbott
and removed request for
a team
|
Oops, test failures appeared with my last change. I will investigate and put back into review when they are passing. |
cahrens
changed the title
|
jenkins retest |
|
jenkins retest |
core/src/main/scala/org/broadinstitute/dsde/rawls/billing/GoogleBillingProjectLifecycle.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/org/broadinstitute/dsde/rawls/billing/BillingProfileManagerDAO.scala
Outdated
Show resolved
Hide resolved
core/src/test/scala/org/broadinstitute/dsde/rawls/billing/BillingProfileManagerDAOSpec.scala
Show resolved
Hide resolved
...src/test/scala/org/broadinstitute/dsde/rawls/billing/GoogleBillingProjectLifecycleSpec.scala
Outdated
Show resolved
Hide resolved
| mock[MultiCloudWorkspaceConfig], | ||
| testContext | ||
| ), | ||
| Duration.Inf |
There was a problem hiding this comment.
Do we want to allow an infinite wait here?
There was a problem hiding this comment.
We seem to do this all over the tests. I honestly don't know what the alternative would be.
There was a problem hiding this comment.
(Not a blocker to merge) In places where we have to wait in a test, it's usually preferable to set some kind of timeout, considering our expectations on how long something will take plus a sizable buffer to avoid needlessly flaky tests. That way a test will fail fast(er) if it's stuck, and go onto a retry if those are configured.
There was a problem hiding this comment.
@marctalbott do you happen to know the best practice for Scala unittests? I think Duration.Inf is what we commonly use.
There was a problem hiding this comment.
In this case, everything is mocked out, so I have no problem with using Duration.Inf here. The Await is a means to get the value out of the Future -- the timeout won't have any impact as we're not really waiting for anything in this test since Mockito will return all Futures quickly.
As for other uses of Duration.Inf in Rawls unit tests, I'm sure there are places we're misusing it. Still, I'm not that worried about those places since they're all unit tests and in theory won't be waiting for long. I'd be very concerned if we had infinite waits in our integration tests.
...src/test/scala/org/broadinstitute/dsde/rawls/billing/GoogleBillingProjectLifecycleSpec.scala
Outdated
Show resolved
Hide resolved
...src/test/scala/org/broadinstitute/dsde/rawls/billing/GoogleBillingProjectLifecycleSpec.scala
Show resolved
Hide resolved
...src/test/scala/org/broadinstitute/dsde/rawls/billing/GoogleBillingProjectLifecycleSpec.scala
Outdated
Show resolved
Hide resolved
|
Converting to draft so we don't get review reminders about this (or accidentally merge it). |
* wip * bump bpm * fix patch reflection error * eat exceptions from BPM * update/add tests * add retries on 5xx errors from BPM * emit sentry event when BPM fails to update * update exception sent to sentry * fix tests * add BPM DAO tests * catch more exceptions
marctalbott
changed the title
Ticket: https://broadworkbench.atlassian.net/browse/WOR-849
This is the first in a group of tickets that will need to merge together, so keep on a feature branch.
Testing: in addition to the unit tests, I created a GCP billing project using my locally running Rawls. We don't return the billing profile ID when fetching billing project details, but I was able to check in the debugger that we have persisted the ID and thus can fetch the billing profile (which we do to add policies and Azure MRG information).
PR checklist
model/, then you should publish a new officialrawls-modeland updaterawls-modelin Orchestration's dependencies.