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

[WOR-1226] For Azure billing projects, return the region of the landing zone #2734

Merged
merged 4 commits into from
Feb 14, 2024

Conversation

cahrens
Copy link
Contributor

@cahrens cahrens commented Feb 12, 2024

Ticket: https://broadworkbench.atlassian.net/browse/WOR-1226

This passes through the region for Azure landing zones in the billing projects response (both list and single billing project signatures). Note that regions have been backfilled yet for existing landing zones, so old landing zones will temporarily return an empty string for region.

Response when running this branch locally against dev databases:

{
        "cloudPlatform": "AZURE",
        "invalidBillingAccount": false,
        "landingZoneId": "43217cdd-8037-463c-b119-3e8bdf6b0b97",
        "managedAppCoordinates": {
            "managedResourceGroupId": "mrg-terra-dev-previ-20240212144940",
            "subscriptionId": "df547342-9cfd-44ef-a6dd-df0ede32f1e3",
            "tenantId": "fad90753-2022-4456-9b0a-c7e5b934e408"
        },
        "projectName": "CARFeb12West",
        "protectedData": false,
        "region": "westus2", <-- newly created landing zone
        "roles": [
            "Owner"
        ],
        "status": "Ready"
    },
    {
        "cloudPlatform": "AZURE",
        "invalidBillingAccount": false,
        "landingZoneId": "2afef35f-991f-41ed-8dde-12a846ab9dd0",
        "managedAppCoordinates": {
            "managedResourceGroupId": "mrg-terra-dev-previ-20230119110658",
            "subscriptionId": "df547342-9cfd-44ef-a6dd-df0ede32f1e3",
            "tenantId": "fad90753-2022-4456-9b0a-c7e5b934e408"
        },
        "projectName": "CARJan19bemis",
        "protectedData": false,
        "region": "", <-- old landing zone that hasn't been backfilled
        "roles": [
            "Owner"
        ],
        "status": "Ready"
    },
    {
        "cloudPlatform": "AZURE",
        "invalidBillingAccount": false,
        "landingZoneId": "5723f447-aaca-47e7-9b55-07c349cd7544",
        "managedAppCoordinates": {
            "managedResourceGroupId": "mrg-terra-dev-previ-20230607135519",
            "subscriptionId": "df547342-9cfd-44ef-a6dd-df0ede32f1e3",
            "tenantId": "fad90753-2022-4456-9b0a-c7e5b934e408"
        },
        "message": "Landing Zone deletion failed: Landing Zone 5723f447-aaca-47e7-9b55-07c349cd7544 not found.",
        "projectName": "CARJan7Local",
        "protectedData": false, <-- No region field because landing zone no longer exists
        "roles": [
            "Owner"
        ],
        "status": "DeletionFailed"
    },
  

---

**PR checklist**

- [ ] Include the JIRA issue number in the PR description and title
- [ ] Make sure Swagger is updated if API changes
  - [ ] **...and Orchestration's Swagger too!**
- [ ] If you changed anything in `model/`, then you should [publish a new official `rawls-model`](https://github.com/broadinstitute/rawls/blob/develop/README.md#publish-rawls-model) and update `rawls-model` in [Orchestration's dependencies](https://github.com/broadinstitute/firecloud-orchestration/blob/develop/project/Dependencies.scala).
- [ ] Get two thumbsworth of PR review
- [ ] Verify all tests go green, including CI tests
- [ ] **Squash commits and merge** to develop (branches are automatically deleted after merging)
- [ ] Inform other teams of any substantial changes via Slack and/or email

@cahrens cahrens marked this pull request as ready for review February 13, 2024 16:59
@cahrens cahrens requested review from a team, blakery and okotsopoulos and removed request for a team February 13, 2024 17:04
@cahrens cahrens marked this pull request as draft February 13, 2024 18:37
@cahrens cahrens changed the title Update WSM client version (WOR-1226). [WOR-1226] For Azure billing projects, return the region of the landing zone Feb 13, 2024
@cahrens
Copy link
Contributor Author

cahrens commented Feb 13, 2024

jenkins retest

1 similar comment
@cahrens
Copy link
Contributor Author

cahrens commented Feb 14, 2024

jenkins retest

@cahrens cahrens marked this pull request as ready for review February 14, 2024 13:49
Await.result(userService.listBillingProjectsV2(), Duration.Inf) should contain theSameElementsAs expected
}

it should "map handle the landing zone being missing in BPM project" in {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I understand what this means since I know the scope of changes in this PR , but the wording is kind of confusing otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, it was a copy/paste error.

val landingZoneRegion = project.landingZoneId match {
case Some(landingZoneId) =>
Try(workspaceManagerDao.getLandingZone(UUID.fromString(landingZoneId), ctx)) match {
case Success(landingZone) => Some(landingZone.getRegion)
Copy link
Contributor

Choose a reason for hiding this comment

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

If you use Option(landingZone.getRegion), you'll get the result of None if it's absent/null, and Some(region) if present. That's probably preferable to the behavior as written here, which will give Some(null)` if the region is not present, since nulls should generally be avoided in scala (avoiding null anywhere is ideal IMO, but scala has explicit conventions around discouraging null values).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, definitely better. Thank you!

@cahrens
Copy link
Contributor Author

cahrens commented Feb 14, 2024

jenkins retest

@cahrens cahrens requested a review from blakery February 14, 2024 15:23
@cahrens
Copy link
Contributor Author

cahrens commented Feb 14, 2024

@blakery I addressed your feedback (and got a passing swatomation run!)

@cahrens cahrens merged commit 3a2b084 into develop Feb 14, 2024
13 checks passed
@cahrens cahrens deleted the WOR-1226 branch February 14, 2024 19:35
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