[RW-11461] UpdateAppConfig to change autodelete settings #4575

jmthibault79 · 2024-05-20T21:58:46Z

Jira ticket: https://precisionmedicineinitiative.atlassian.net/browse/RW-11461 (requires PMI-OPS Jira access)

Includes the work from the previous draft #4232

Summary of changes

Adds a new endpoint "Update App Config" PATCH /api/google/v1/apps/{googleProject}/{appName} which allows users to modify the two autodelete app config fields: autodeleteEnabled and autodeleteThreshold. Requests can contain changes to one field or both (or neither) as long as (a) any specified threshold is a positive integer and (b) any requests which enable autodelete without specifying the threshold correspond to apps with pre-existing valid thresholds in the DB.

More details here: https://docs.google.com/document/d/1eWfY8PQoDKVOdIeREuEC9ObLhXlsTtG2ugLwuqov81k/edit#bookmark=id.n9sphs8lpm4l

What

Why

AoU RWB users want the ability to modify and enable/disable the autodelete settings of running apps. Currently they must delete the apps instead, creating them again with the desired settings

Testing these changes

I deployed this change to a BEE and created a Galaxy app in the BEE's Terra UI. I then observed that the app state (as seen via GET app) responded to my PATCH changes as expected.

What to test

Who tested and where

This change is covered by automated tests
- NB: Rerun automation tests on this PR by commenting jenkins retest or jenkins multi-test.
I validated this change
Primary reviewer validated this change
I validated this change in the dev environment

codecov · 2024-05-20T22:19:57Z

Codecov Report

Attention: Patch coverage is 75.55556% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 73.68%. Comparing base (6baab71) to head (b995b35).

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #4575   +/-   ##
========================================
  Coverage    73.68%   73.68%           
========================================
  Files          159      159           
  Lines        14724    14764   +40     
  Branches      1166     1133   -33     
========================================
+ Hits         10849    10879   +30     
- Misses        3875     3885   +10

Files	Coverage Δ
...dsde/workbench/leonardo/http/appRoutesModels.scala	`0.00% <ø> (ø)`
...tute/dsde/workbench/leonardo/db/AppComponent.scala	`97.93% <100.00%> (+0.02%)`	⬆️
...ch/leonardo/http/service/LeoAppServiceInterp.scala	`87.15% <100.00%> (+0.60%)`	⬆️
...e/dsde/workbench/leonardo/http/api/AppRoutes.scala	`81.05% <15.38%> (-10.42%)`	⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6baab71...b995b35. Read the comment docs.

jmthibault79 · 2024-05-22T13:53:30Z

...est/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterpSpec.scala

  }

-  "checkIfAppCreationIsAllowedAndIsAoU" should "enable IntraNodeVisibility if customApp check is disabled" in {
+  "checkIfAppCreationIsAllowed" should "enable IntraNodeVisibility if customApp check is disabled" in {


not related to this PR, but I happened to notice that this did not match the method name

jmthibault79 · 2024-05-22T14:43:34Z

...main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala

+      )
+
+      tags = Map("appType" -> appResult.app.appType.toString)
+      _ <- metrics.incrementCounter("updateAppConfig", 1, tags)


I noticed that the counter is sometimes incremented at the Service level and sometimes at the Routes level. Unclear which to choose.

core/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/appRoutesModels.scala

jmthibault79 · 2024-05-22T17:55:03Z

Moving back to Draft while I have a think about logic/semantics

http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppRoutes.scala

http/src/main/resources/swagger/api-docs.yaml

http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppRoutes.scala

...main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala

core/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/appRoutesModels.scala

http/src/main/resources/swagger/api-docs.yaml

...main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala

jmthibault79 · 2024-05-24T18:53:27Z

if we use a custom type for autodeleteThreshold

@Qi77Qi I made an attempt but I was unable to get it all the way working #4583

jmthibault79 · 2024-05-28T17:57:48Z

...main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala

    )
  }

+  private[service] def validateAutodelete(autodeleteEnabled: Boolean,


not sure where this should be placed. thoughts?

jmthibault79 · 2024-05-28T18:52:10Z

...main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala

-                       BadRequestException("autodeleteThreshold should be a positive value", Some(ctx.traceId))
-      )
+      autodeleteEnabled = req.autodeleteEnabled.getOrElse(false)
+      _ <- validateAutodelete(autodeleteEnabled, req.autodeleteThreshold, ctx.traceId)


note that this makes the Create App validation more strict, to match

jmthibault79 · 2024-05-28T18:56:21Z

...main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala

+      appQuery
+        .updateAutodeleteThreshold(appId, Some(threshold))
+    )
+  } yield ()).transaction


Don't want to commit one without the other!

Example: the app had a 1-hour threshold, but it was disabled. Now the user wants to enable with a 10-hour threshold. If only the first change goes through, the app has a surprise active 1-hour threshold.

jmthibault79 force-pushed the joel/RW-11461 branch from 5f2fabb to 9f97d25 Compare May 21, 2024 22:28

jmthibault79 commented May 22, 2024

View reviewed changes

jmthibault79 marked this pull request as ready for review May 22, 2024 14:46

jmthibault79 changed the title ~~[RW-11461] WIP user app update autodelete~~ [RW-11461] UpdateAppConfig to change autodelete settings May 22, 2024

Qi77Qi reviewed May 22, 2024

View reviewed changes

core/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/appRoutesModels.scala Outdated Show resolved Hide resolved

jmthibault79 marked this pull request as draft May 22, 2024 17:54

jmthibault79 force-pushed the joel/RW-11461 branch from e2acd3a to 5c60e15 Compare May 23, 2024 20:34

jmthibault79 marked this pull request as ready for review May 23, 2024 21:24

jmthibault79 requested a review from Qi77Qi May 23, 2024 21:26

jmthibault79 commented May 23, 2024

View reviewed changes

http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/api/AppRoutes.scala Outdated Show resolved Hide resolved

jmthibault79 requested a review from LizBaldo May 23, 2024 22:11

LizBaldo reviewed May 24, 2024

View reviewed changes

Qi77Qi reviewed May 24, 2024

View reviewed changes

cpate4 reviewed May 24, 2024

View reviewed changes

http/src/main/resources/swagger/api-docs.yaml Outdated Show resolved Hide resolved

...main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/LeoAppServiceInterp.scala Outdated Show resolved Hide resolved

jmthibault79 force-pushed the joel/RW-11461 branch from dbe77ac to 30b3b72 Compare May 24, 2024 20:56

jmthibault79 commented May 28, 2024

View reviewed changes

jmthibault79 requested review from LizBaldo and Qi77Qi May 28, 2024 19:20

jmthibault79 force-pushed the joel/RW-11461 branch from 4d21ab8 to b1cd3ad Compare May 28, 2024 20:26

cpate4 approved these changes May 28, 2024

View reviewed changes

Qi77Qi approved these changes May 29, 2024

View reviewed changes

jmthibault79 force-pushed the joel/RW-11461 branch 2 times, most recently from 510a802 to caaee02 Compare May 30, 2024 15:18

From original PR #4232

8c71074

jmthibault79 added 20 commits May 31, 2024 09:21

UpdateAppConfig and tests

d989c2e

foldSpan

b8ac4af

add test

0929dfa

logic fix

4b41ea8

formatting

117fff4

UpdateAppConfigRequest description

3ef3bac

update validateUpdateAppConfigRequest comment

56d8aa2

traverse

2476ced

rename UpdateAppConfig to UpdateApp

cbcb60c

lint

88f55ef

comment

1be057c

rv foldspan

26aba7d

WIP tests run

fb796b6

common logic

0922098

oops

eebf647

rm spurious

5383d07

spacing

0ac9141

rename

fff4483

getUpdateAppTransaction

102e8af

trivial change to re-trigger tests

b995b35

jmthibault79 force-pushed the joel/RW-11461 branch from fe3810e to b995b35 Compare May 31, 2024 13:21

jmthibault79 merged commit 823027a into develop May 31, 2024

jmthibault79 deleted the joel/RW-11461 branch May 31, 2024 14:29

jmthibault79 mentioned this pull request May 31, 2024

[RW-11461] Use a value class for AutodeleteThreshold #4583

Merged

4 tasks

[RW-11461] UpdateAppConfig to change autodelete settings #4575

[RW-11461] UpdateAppConfig to change autodelete settings #4575

Uh oh!

Conversation

jmthibault79 commented May 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

What

Why

Testing these changes

What to test

Who tested and where

Uh oh!

codecov bot commented May 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jmthibault79 May 22, 2024

Choose a reason for hiding this comment

Uh oh!

jmthibault79 May 22, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmthibault79 commented May 22, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmthibault79 commented May 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmthibault79 May 28, 2024

Choose a reason for hiding this comment

Uh oh!

jmthibault79 May 28, 2024

Choose a reason for hiding this comment

Uh oh!

jmthibault79 May 28, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jmthibault79 commented May 20, 2024 •

edited

Loading

codecov bot commented May 20, 2024 •

edited

Loading

jmthibault79 commented May 24, 2024 •

edited

Loading