[WM-2427] Remove ability for Cromwell appTypes to be deleted #4592

kpierre13 · 2024-01-16T21:15:05Z

Jira Ticket: https://broadworkbench.atlassian.net/browse/[WM-2427]

Summary of changes:

What

Removing Cromwell app types from being displayed on the Clusters page

Why

Currently, deleting a Cromwell app type will put a workspace into an error state. To avoid users from deleting these apps and erroring their workspace, we want to stop a user from being able to delete, but still show them information regarding their apps.

Testing strategy

Unit tests

Before:

After:

LizBaldo

Do I understand correctly that the only way to delete cromwell app types on either cloud is through deleting the workspace?

src/analysis/Environments/Environments.ts

kpierre13 · 2024-01-19T18:22:20Z

Do I understand correctly that the only way to delete cromwell app types on either cloud is through deleting the workspace?

That's my understanding. I'm not totally clear about the fine details, but since reconnecting to a previously running app state is not supported, manually deleting from the clusters page throws the associated workspace into a weird state.

jdcanas · 2024-01-19T20:07:52Z

src/analysis/Environments/Environments.ts

-      leoAppData.listWithoutProject(listArgs, { signal }),
+      // Excluding cromwell/workflows app types for now, as trying to delete these app types puts associated workspace in an error state
+      (
+        await leoAppData.listWithoutProject(listArgs, { signal })


Hmm... I don't particularly like this approach, it is going the opposite direction we are trying to in terms of UI architecture; adding business logic to visual components. Furthermore, just because a resource is not deletable does not necessarily mean we don't want to show it on this page.

I'd advise waiting until this PR is merged, and extending environmentPermissions with the already existing function isResourceDeletable.

At the very least, if you need to move this forward ASAP, I'd recommend leveraging isResourceDeletable to centralize the logic. I believe the delete buttons already leverage this function, so adding the logic there should be sufficient.

cc @msilva-broad , so you know I'm advising someone extend work you have in progress

jdcanas · 2024-01-23T22:02:29Z

src/analysis/Environments/Environments.models.ts

@@ -22,5 +22,9 @@ export interface LeoResourcePermissionsProvider {
  canPauseResource: (resource: App | ListRuntimeItem) => boolean;
 }

+export interface CanDeleteProvider {
+  canBeDeleted: (resource: App) => boolean;


this can live in the LeoResourcePermissionProvider and shouldn't need its own interface.

jdcanas · 2024-01-23T22:03:03Z

src/analysis/Environments/Environments.models.ts

@@ -22,5 +22,9 @@ export interface LeoResourcePermissionsProvider {
  canPauseResource: (resource: App | ListRuntimeItem) => boolean;
 }

+export interface CanDeleteProvider {
+  canBeDeleted: (resource: App) => boolean;


I'd also call it canDeleteApp to be consistent with other interface method names

jdcanas · 2024-01-23T22:03:40Z

src/analysis/Environments/Environments.test.ts

@@ -524,6 +524,51 @@ describe('Environments', () => {
      expect(getTextContentForColumn(fourthAppRow, 8)).toBe(makeCompleteDate(azureApp2.auditInfo.dateAccessed));
    });

+    it('Renders Cromwell apps with disabled delete', async () => {


thanks for adding a detailed test!

agreed! this is a great looking test.

jdcanas · 2024-01-23T22:04:56Z

src/pages/EnvironmentsPage/environmentsPermissions.ts

@@ -21,6 +22,14 @@ const makePermissionsProvider = (userEmailGetter: () => string): LeoResourcePerm
  return permissionsProvider;
 };

+export const makeCromwellAppsNotDeletable = (): CanDeleteProvider => {


should move to the above makePermissionsProvider once you consolidate CanDeleteProvider into LeoResourcePermissionsProvider

jdcanas · 2024-01-23T22:19:52Z

src/analysis/Environments/DeleteButton.ts

@@ -18,19 +21,28 @@ export interface DeleteButtonProps {
 export const DeleteButton = (props: DeleteButtonProps): ReactNode => {
  const { resource, onClick } = props;
  const resourceType = isApp(resource) ? 'app' : 'runtime';
-  const isDeletable = isResourceDeletable(resourceType, resource);
+  // Casting resource to type App to access the appType property
+  const castToApp = resource as App;


We shouldn't need to cast.

It sounds like we want to function canBeDeleted to take resource: DecoratedComputeResource and check isResourceDeletable and the logic you have in !Object.keys(cromwellAppToolLabels).includes(resource.appType);. It will need to do something like the following

if (isApp(resource)) { // access appType here } else { //unimplemented exception, other resources, aka runtimes, not implemented yet }

Bonus points if you can consolidate it with the canDeleteDisk function but you'd need to re-wire the type here from DecoratedComputeResource to Resource.

Sorry if this is a bit confusing, definitely down to pair and help you get this through. You've got all the pieces in place here though, a great second iteration! tl;dr, we should not need to cast in the DeleteButton, the permissions provider should handle the guts of isDeletable and can take the resource directly and figure it out from there.

I think I see what you're saying. Let me take a stab at this implementation, and I'll reach out if I get stuck. Thank you for all of the feedback!

Also, just a quick clarification, are you advocating for calling isResourceDeletable from within canBeDeleted?

I left that intentionally vague 😅 . I think the permissionsProvider ultimately was designed to do one thing only, tell you if you have permissions to delete something (which relates to IAM logic).

I think we are encountering the need for another provider, one that can tell you whether a resource is in a state (business logic or otherwise) that allows a specific action.

In the scope of your PR, I could see you doing any of the following, as long as we don't straddle between multiple approaches:

extending the permissionsProvider with this isResourceDeletable functionality (seems the least desirable)

extending the utility function isResourceDeletable to include your cromwell-specific logic

creating a new provider that the delete button consumes that wraps the permissionsProvider and includes the utility function (isResourceDeletable) logic + your new cromwell app logic

Sorry if I'm a bit scattered here, but trying to move towards an ideal architecture with old code through incremental improvements is a bit tricky 😅 . I'd vote for the third above, but I understand if you would rather go with something along the lines of #2 to not expand the scope of your PR too much

No worries! I don't think it'll be too much of a lift to create a new wrapper provider, so I'll do my best to try and implement option #3 here. I'll reach out in Slack if things start to get a little wonky in my understanding.

jdcanas · 2024-02-06T20:28:54Z

src/analysis/Environments/DeleteButton.ts

  const resourceType = isApp(resource) ? 'app' : 'runtime';
-  const isDeletable = isResourceDeletable(resourceType, resource);
+  const isDeletable =
+    resourceType === 'app' ? permissions.canDeleteApp(resource) : permissions.canDeleteResource(resourceType, resource);


We should use isApp here instead of a string to determine typing and enforce type safety. This will let you change canDeleteApp to actually take an App as well.

See the TODO in isResourceDeletable in the ts file, we shouldn't need to pass resourceType to canDeleteResource, we can ask the type of the resource directly.

jdcanas · 2024-02-06T20:29:20Z

src/analysis/Environments/DeleteButton.ts

  onClick: (resource: DecoratedComputeResource) => void;
 }

 export const DeleteButton = (props: DeleteButtonProps): ReactNode => {
-  const { resource, onClick } = props;
+  const { resource, permissions, onClick } = props;
  const resourceType = isApp(resource) ? 'app' : 'runtime';


this can be deleted

jdcanas · 2024-02-06T20:30:46Z

src/pages/EnvironmentsPage/environmentsPermissions.ts

    },
    canPauseResource: (resource: App | ListRuntimeItem) => {
      const currentUserEmail = userEmailGetter();
      const creator = getCreatorForCompute(resource);
      return currentUserEmail === creator;
    },
+    canDeleteApp: (resource: DecoratedComputeResource) => {


this should take an App?

jdcanas · 2024-02-06T20:31:20Z

src/pages/EnvironmentsPage/environmentsPermissions.ts

+      }
+    },
+    canDeleteResource: (resourceType, resource: App | PersistentDisk | Runtime) => {
+      return isResourceDeletable(resourceType, resource);


See previous comment, we should not be passing resourceType around, the resources have types!

jdcanas

Looks mostly there! Just a bit of cleanup needed (with inline comments)

resourceType should not be used anywhere, its a code smell to be passing around a string for the type when we can inspect the resource itself with questions like isApp.

Additionally, canDeleteApp should take an App

jdcanas · 2024-02-07T18:26:54Z

src/analysis/utils/resource-utils.ts

-      ['app', () => ['unspecified', 'running', 'error']],
-      ['disk', () => ['failed', 'ready']],
+    Utils.cond(
+      [isApp(resource), () => ['unspecified', 'running', 'error']],


Thanks a lot for cleaning this up! You can remove the TODO on line 15 now!

jdcanas

Great, thanks for cleaning this up! One small nit about removing a TODO you addressed.

Eventually, I would like to see canDeleteResource call canDeleteApp so the call site doesn't need to choose which function to call, but I don't feel super strongly about doing that this iteration since you've already left the code in a much better place than when you started

kpierre13 · 2024-02-07T18:44:45Z

Great, thanks for cleaning this up! One small nit about removing a TODO you addressed.

Eventually, I would like to see canDeleteResource call canDeleteApp so the call site doesn't need to choose which function to call, but I don't feel super strongly about doing that this iteration since you've already left the code in a much better place than when you started

I agree with your point. I think the goal is for canDeleteApp to be temporary. Once there is handling of deleting Cromwell-type apps, canDeleteResource should be the only decider for all resource types.

sonarcloud · 2024-02-07T19:12:37Z

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
76.2% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

marctalbott

Workspaces changes look good!

kpierre13 added 3 commits January 16, 2024 16:12

filter out cromwell app types; update test

01837c7

cleanup

f21115f

Commenting out azure tests to be used again in the future

f94f52e

broadbot temporarily deployed to pr-5 January 16, 2024 22:03 Inactive

refactor

cc9aa48

broadbot temporarily deployed to pr-5 January 16, 2024 22:23 Inactive

kpierre13 marked this pull request as ready for review January 17, 2024 15:20

kpierre13 requested a review from a team as a code owner January 17, 2024 15:20

LizBaldo reviewed Jan 19, 2024

View reviewed changes

src/analysis/Environments/Environments.ts Outdated Show resolved Hide resolved

excluding cromwell app types

4268574

kpierre13 requested a review from LizBaldo January 19, 2024 18:22

broadbot temporarily deployed to pr-5 January 19, 2024 18:32 Inactive

jdcanas requested changes Jan 19, 2024

View reviewed changes

Merge branch 'dev' into wm-2427_remove_cromwell_app_instances_kp

d553483

broadbot temporarily deployed to pr-5 January 22, 2024 19:53 Inactive

using environmentPermissions logic

b50e128

kpierre13 changed the title ~~[WM-2427] Remove Cromwell app types from being displayed to users~~ [WM-2427] Remove ability for Cromwell appTypes to be deleted Jan 23, 2024

add a test; revert other tests

8dabb33

kpierre13 requested a review from jdcanas January 23, 2024 21:01

kpierre13 added 3 commits January 23, 2024 16:29

change type of resource

dcdc6a4

cast to access appType

fcb1fbc

update tests

5e09b16

jdcanas reviewed Jan 23, 2024

View reviewed changes

kpierre13 added 2 commits January 24, 2024 09:42

wip

f870837

new permission and state provider

6d16f23

kpierre13 and others added 3 commits February 1, 2024 14:31

renaming PR comment

967a632

stronger type safety in test

2466b5a

Merge branch 'dev' into wm-2427_remove_cromwell_app_instances_kp

30e520a

broadbot temporarily deployed to pr-5 February 5, 2024 17:46 Inactive

kpierre13 requested a review from msilva-broad February 5, 2024 18:30

kpierre13 and others added 2 commits February 6, 2024 10:49

Merge branch 'dev' into wm-2427_remove_cromwell_app_instances_kp

c6b70e1

cleanup

b68ef1b

broadbot temporarily deployed to pr-5 February 6, 2024 16:07 Inactive

jdcanas reviewed Feb 6, 2024

View reviewed changes

jdcanas requested changes Feb 6, 2024

View reviewed changes

PR comments

60641fc

kpierre13 requested a review from jdcanas February 7, 2024 16:48

jdcanas reviewed Feb 7, 2024

View reviewed changes

jdcanas approved these changes Feb 7, 2024

View reviewed changes

CI failures

9125f5b

kpierre13 requested a review from a team as a code owner February 7, 2024 18:38

kpierre13 requested a review from marctalbott February 7, 2024 18:38

broadbot temporarily deployed to pr-5 February 7, 2024 18:52 Inactive

Merge branch 'dev' into wm-2427_remove_cromwell_app_instances_kp

b3988d1

broadbot temporarily deployed to pr-5 February 7, 2024 19:12 Inactive

marctalbott approved these changes Feb 7, 2024

View reviewed changes

kpierre13 added this pull request to the merge queue Feb 7, 2024

Merged via the queue into dev with commit dc3d28b Feb 7, 2024
7 checks passed

kpierre13 deleted the wm-2427_remove_cromwell_app_instances_kp branch February 7, 2024 20:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WM-2427] Remove ability for Cromwell appTypes to be deleted #4592

[WM-2427] Remove ability for Cromwell appTypes to be deleted #4592

kpierre13 commented Jan 16, 2024 •

edited

Loading

LizBaldo left a comment

kpierre13 commented Jan 19, 2024

jdcanas Jan 19, 2024 •

edited

Loading

jdcanas Jan 23, 2024

jdcanas Jan 23, 2024

jdcanas Jan 23, 2024

msilva-broad Feb 1, 2024

jdcanas Jan 23, 2024

jdcanas Jan 23, 2024 •

edited

Loading

kpierre13 Jan 23, 2024

kpierre13 Jan 23, 2024

jdcanas Jan 23, 2024 •

edited

Loading

jdcanas Jan 23, 2024

kpierre13 Jan 23, 2024

jdcanas Feb 6, 2024 •

edited

Loading

jdcanas Feb 6, 2024

jdcanas Feb 6, 2024

jdcanas Feb 6, 2024

jdcanas left a comment •

edited

Loading

jdcanas Feb 7, 2024

jdcanas left a comment

kpierre13 commented Feb 7, 2024

sonarcloud bot commented Feb 7, 2024

marctalbott left a comment

[WM-2427] Remove ability for Cromwell appTypes to be deleted #4592

[WM-2427] Remove ability for Cromwell appTypes to be deleted #4592

Conversation

kpierre13 commented Jan 16, 2024 • edited Loading

Jira Ticket: https://broadworkbench.atlassian.net/browse/[WM-2427]

Summary of changes:

What

Why

Testing strategy

LizBaldo left a comment

Choose a reason for hiding this comment

kpierre13 commented Jan 19, 2024

jdcanas Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdcanas Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdcanas Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdcanas Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdcanas left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdcanas left a comment

Choose a reason for hiding this comment

kpierre13 commented Feb 7, 2024

sonarcloud bot commented Feb 7, 2024

Quality Gate passed

marctalbott left a comment

Choose a reason for hiding this comment

kpierre13 commented Jan 16, 2024 •

edited

Loading

jdcanas Jan 19, 2024 •

edited

Loading

jdcanas Jan 23, 2024 •

edited

Loading

jdcanas Jan 23, 2024 •

edited

Loading

jdcanas Feb 6, 2024 •

edited

Loading

jdcanas left a comment •

edited

Loading