-
Notifications
You must be signed in to change notification settings - Fork 21
[RW-11461] UpdateAppConfig to change autodelete settings #4575
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
Changes from all commits
8c71074
d989c2e
b8ac4af
0929dfa
4b41ea8
117fff4
3ef3bac
56d8aa2
2476ced
cbcb60c
88f55ef
1be057c
26aba7d
fb796b6
0922098
eebf647
5383d07
0ac9141
fff4483
102e8af
b995b35
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,6 +29,7 @@ import org.broadinstitute.dsde.workbench.leonardo.JsonCodec._ | |
| import org.broadinstitute.dsde.workbench.leonardo.SamResourceId._ | ||
| import org.broadinstitute.dsde.workbench.leonardo.config._ | ||
| import org.broadinstitute.dsde.workbench.leonardo.dao.{WsmApiClientProvider, WsmDao} | ||
| import org.broadinstitute.dsde.workbench.leonardo.db.DBIOInstances.dbioInstance | ||
| import org.broadinstitute.dsde.workbench.leonardo.db.KubernetesServiceDbQueries.getActiveFullAppByWorkspaceIdAndAppName | ||
| import org.broadinstitute.dsde.workbench.leonardo.db._ | ||
| import org.broadinstitute.dsde.workbench.leonardo.http.service.LeoAppServiceInterp.{ | ||
|
|
@@ -669,6 +670,48 @@ final class LeoAppServiceInterp[F[_]: Parallel](config: AppServiceConfig, | |
| .raiseError[Unit](AppNotFoundByWorkspaceIdException(workspaceId, appName, ctx.traceId, "permission denied")) | ||
| } yield GetAppResponse.fromDbResult(app, Config.proxyConfig.proxyUrlBase) | ||
|
|
||
| private def getUpdateAppTransaction(appId: AppId, validatedChanges: UpdateAppRequest): F[Unit] = (for { | ||
| _ <- validatedChanges.autodeleteEnabled.traverse(enabled => | ||
| appQuery | ||
| .updateAutodeleteEnabled(appId, enabled) | ||
| ) | ||
|
|
||
| // note: does not clear the threshold if None. This only sets defined thresholds. | ||
| _ <- validatedChanges.autodeleteThreshold.traverse(threshold => | ||
| appQuery | ||
| .updateAutodeleteThreshold(appId, Some(threshold)) | ||
| ) | ||
| } yield ()).transaction | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| override def updateApp(userInfo: UserInfo, cloudContext: CloudContext.Gcp, appName: AppName, req: UpdateAppRequest)( | ||
| implicit as: Ask[F, AppContext] | ||
| ): F[Unit] = | ||
| for { | ||
| ctx <- as.ask | ||
|
|
||
| // throw 403 if no project-level permission | ||
| hasProjectPermission <- authProvider.isUserProjectReader(cloudContext, userInfo) | ||
| _ <- F.raiseWhen(!hasProjectPermission)(ForbiddenError(userInfo.userEmail, Some(ctx.traceId))) | ||
|
|
||
| appOpt <- KubernetesServiceDbQueries.getActiveFullAppByName(cloudContext, appName).transaction | ||
| appResult <- F.fromOption(appOpt, | ||
| AppNotFoundException(cloudContext, appName, ctx.traceId, "No active app found in DB") | ||
| ) | ||
|
|
||
| _ <- metrics.incrementCounter("updateApp", 1, Map("appType" -> appResult.app.appType.toString)) | ||
|
|
||
| // throw 404 if no UpdateApp permission | ||
| listOfPermissions <- authProvider.getActions(appResult.app.samResourceId, userInfo) | ||
| hasPermission = listOfPermissions.toSet.contains(AppAction.UpdateApp) | ||
jmthibault79 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| _ <- F.raiseWhen(!hasPermission)(AppNotFoundException(cloudContext, appName, ctx.traceId, "Permission Denied")) | ||
|
|
||
| // confirm that the combination of the request and the existing DB values result in a valid configuration | ||
| resolvedAutodeleteEnabled = req.autodeleteEnabled.getOrElse(appResult.app.autodeleteEnabled) | ||
| resolvedAutodeleteThreshold = req.autodeleteThreshold.orElse(appResult.app.autodeleteThreshold) | ||
| _ <- F.fromEither(validateAutodelete(resolvedAutodeleteEnabled, resolvedAutodeleteThreshold, ctx.traceId)) | ||
| _ <- getUpdateAppTransaction(appResult.app.id, req) | ||
| } yield () | ||
|
|
||
| override def createAppV2(userInfo: UserInfo, workspaceId: WorkspaceId, appName: AppName, req: CreateAppRequest)( | ||
| implicit as: Ask[F, AppContext] | ||
| ): F[Unit] = | ||
|
|
@@ -1429,13 +1472,9 @@ final class LeoAppServiceInterp[F[_]: Parallel](config: AppServiceConfig, | |
| if (req.appType == AppType.Allowed) | ||
| Some(config.leoKubernetesConfig.allowedAppConfig.numOfReplicas) | ||
| else None | ||
| autodeleteEnabled = req.autodeleteEnabled.getOrElse(false) | ||
| autodeleteThreshold = req.autodeleteThreshold.getOrElse(0) | ||
|
|
||
| _ <- Either.cond(!(autodeleteEnabled && autodeleteThreshold <= 0), | ||
| (), | ||
| BadRequestException("autodeleteThreshold should be a positive value", Some(ctx.traceId)) | ||
| ) | ||
| autodeleteEnabled = req.autodeleteEnabled.getOrElse(false) | ||
| _ <- validateAutodelete(autodeleteEnabled, req.autodeleteThreshold, ctx.traceId) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note that this makes the Create App validation more strict, to match |
||
| } yield SaveApp( | ||
| App( | ||
| AppId(-1), | ||
|
|
@@ -1469,6 +1508,27 @@ final class LeoAppServiceInterp[F[_]: Parallel](config: AppServiceConfig, | |
| ) | ||
| } | ||
|
|
||
| private[service] def validateAutodelete(autodeleteEnabled: Boolean, | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure where this should be placed. thoughts? |
||
| autodeleteThreshold: Option[Int], | ||
| traceId: TraceId | ||
| ): Either[LeoException, Unit] = { | ||
| val invalidThreshold = autodeleteThreshold.exists(_ <= 0) | ||
| val wantEnabledButThresholdMissing = autodeleteEnabled && autodeleteThreshold.isEmpty | ||
|
|
||
| for { | ||
| _ <- Either.cond(!invalidThreshold, | ||
| (), | ||
| BadRequestException("autodeleteThreshold should be a positive value", Some(traceId)) | ||
| ) | ||
|
|
||
| _ <- Either.cond( | ||
| !wantEnabledButThresholdMissing, | ||
| (), | ||
| BadRequestException("when enabling autodelete, an autodeleteThreshold must be present", Some(traceId)) | ||
| ) | ||
| } yield () | ||
| } | ||
|
|
||
| private def getVisibleApps(allClusters: List[KubernetesCluster], userInfo: UserInfo)(implicit | ||
| as: Ask[F, AppContext] | ||
| ): F[Option[List[AppSamResourceId]]] = for { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.