[RW-9614][risk=no] Refactor Cromwell configuration panel #7480
Conversation
Peter-Lavigne
requested review from
a team and
NehaBroad
and removed request for
a team
|
|
| @@ -168,7 +173,7 @@ const PanelMain = fp.flow( | |||
| id='cromwell-cloud-environment-create-button' | |||
| aria-label='cromwell cloud environment create button' | |||
| onClick={onCreate} | |||
| disabled={loading || !!app?.status || creating} | |||
| disabled={loading || !canCreateApp(app) || creating} | |||
There was a problem hiding this comment.
It will be better to move this in a method and add a comment describing the conditions of canCreateApp
There was a problem hiding this comment.
I did some light refactoring and renaming and ended up with this:
const createEnabled = !loadingApps && !creatingCromwellApp && canCreateApp(app);
Should I still add a comment?
| ...listAppsAppResponseSharedDefaults, | ||
| appName: 'all-of-us-2-cromwell-1234', | ||
| appType: AppType.CROMWELL, | ||
| creator: '[email protected]', |
There was a problem hiding this comment.
can we use a mock name?
There was a problem hiding this comment.
Thanks, renamed 👍
There was a problem hiding this comment.
Looks like the API code filters out DELETED apps anyway... so I'm not sure when you'd be seeing one.
private List<UserAppEnvironment> getUserAppEnvironments(
String googleProjectId, AppsApi appsApi, String leonardoAppRole) {
List<LeonardoListAppResponse> listAppResponses =
leonardoRetryHandler.run(
(context) ->
appsApi.listAppByProject(
googleProjectId,
/* labels= */ null,
/* includeDeleted= */ false,
/* includeLabels= */ LeonardoLabelHelper.LEONARDO_APP_LABEL_KEYS,
leonardoAppRole));
return listAppResponses.stream().map(leonardoMapper::toApiApp).collect(Collectors.toList());
}
ui/src/testing/utils.ts
Outdated
| return a1.filter((e) => !a2.includes(e)); | ||
| } | ||
|
|
||
| export const ALL_STATUSES = Object.keys(AppStatus) |
There was a problem hiding this comment.
we have many kinds of status so I'd prefer ALL_APP_STATUSES or allUserAppStatuses or something like that
There was a problem hiding this comment.
Renamed to ALL_GKE_APP_STATUSES 👍
| @@ -36,6 +36,7 @@ import { NotebooksApiStub } from 'testing/stubs/notebooks-api-stub'; | |||
| import { RuntimeApiStub } from 'testing/stubs/runtime-api-stub'; | |||
| import { RuntimesApiStub } from 'testing/stubs/runtimes-api-stub'; | |||
| import { workspaceDataStub } from 'testing/stubs/workspaces'; | |||
| import { ALL_STATUSES, minus } from 'testing/utils'; | |||
|
if its being done in API should we do it again in UI? |
|
Oh, I didn't know we filtered out those GKE apps. I tried to distinguish between This PR no longer fixes a bug, and the multi-app state I described above won't happen. I think these are still valid refactors and test improvements, though. Maybe I should add a comment somewhere so we don't get confused by this again? |
Peter-Lavigne
force-pushed
the
peterlavigne/rw-9614-fix-create-cromwell
branch
from
f2f059b to
8e6dd80
Compare
|
GitHub claimed there was a conflict so I rebased. However, there didn't appear to be a conflict so it seems like a temporary GitHub bug. |
Peter-Lavigne
changed the title
Peter-Lavigne
force-pushed
the
peterlavigne/rw-9614-fix-create-cromwell
branch
from
8e6dd80 to
0e8a393
Compare
|
Good idea, done 👍 |
Description:
Refactors the Cromwell configuration panel to use shared logic for when a GKE app can be created.
Additionally, adds some tests.
Fixes a pre-existing bug where the user cannot create a Cromwell app if their previously-deleted app is in a DELETED state.It's difficult to test or notice this locally since the app is quickly removed from the list of apps returned by the list apps endpoint.Question for the reviewer: When are apps in a "DELETED" state, anyway? What happens if a user has a “deleted” app, then creates another? Would the user have two apps in that scenario, and does our code account for this?PR checklist