feat: add when property to extensions menus

[axel7083]

What does this PR do?

The when property is available on a large range of options, and this PR aims to add support for conditional menus provided by extensions.

Screenshot/screencast of this PR

screenshot	description
	Menu visible only on compose group actions using when: type === compose
	Only visible on running container when: state === RUNNING

What issues does this PR fix or reference?

The accessibles when properties

Depending on which type of ContainerGroup it is, different properties are accessible.

For a standalone container the following properties are accessible in addition of the global context.

standalone

name	type
created	number
displayPort	string
engineId	string
engineName	string
engineType	string
groupInfo	object
hasPublicPort	boolean
icon	string
id	string
image	string
labels	object
name	string
openingUrl	string
ports	array
portsAsString	string
selected	boolean
shortId	string
shortImage	string
startedAt	string
state	string
uptime	string

compose

name	type
containers	array
engineId	string
engineType	string
name	string
status	string
type	string

pod

TODO

Implementation details

The main logic of evaluating the context is delegated to the ContributionActions.svelte component. It was too laborious to manage everything in the ContainerList.svelte.

The ComposeInfoUI and PodInfoUI interface have a new property type which allows to identify the type of ContainerGroup, this has been added since there was no other simple way to be able to distinguish them easily.

An utility context function has been created transformObjectToContext taking any object and transforming it a ContextUI object. This function is generic since, the ContributionActions component does not have any type information about the arguments it receive.

How to test this PR?

[benoitf]

hi @axel7083 thanks for this other PR

I was wondering if we should probably use prefix/suffix on the properties to avoid collisions/separate stuff

for example type and state are too generic

We should describe what is available for given contexts

For example, in container list, probably it should be containerSomething like containerType and containerState

And in image list, `imageSomething, etc.

[axel7083]

hi @axel7083 thanks for this other PR

I was wondering if we should probably use prefix/suffix on the properties to avoid collisions/separate stuff

for example type and state are too generic

We should describe what is available for given contexts

For example, in container list, probably it should be containerSomething like containerType and containerState

And in image list, `imageSomething, etc.

Other field are very generic in the ComposeInfoUI, ContainerInfoUI, ImageInfoUI and PodInfoUI.
I made the ContributionActions component to be able to take a contextPrefix, that will prefix all properties in the context of the given arguments.

For examle, in we can have

<ContributionActions
    args="{[compose]}"
    contextPrefix="ContainerGroup"
    dropdownMenu="{dropdownMenu}"
    contributions="{contributions}"
    onError="{errorCallback}" />

Then the when condition would be ContainerGroup:type === COMPOSE, what do you think ?

[benoitf]

I would say I'm fine with the prefix but I'm unsure if we should use the : notation like ContainerGroupType rather than ContainerGroup:type

I'm just thinking out loud.

So I'm not sure as well about the properties as well.

in the list, we display items so I'm not sure about ContainerGroup prefix

so at the end, is it like containerListItem

[benoitf]

or other ideas

[axel7083]

I would say I'm fine with the prefix but I'm unsure if we should use the : notation like ContainerGroupType rather than ContainerGroup:type

I'm just thinking out loud.

So I'm not sure as well about the properties as well.

in the list, we display items so I'm not sure about ContainerGroup prefix

so at the end, is it like containerListItem

I implemented first the : since in the documentation we can already see it being used.

But if you rather have simple prefix, as you want.

[benoitf]

the : prefix was used for the scope of context properties. (to see properties only in a given context)

so ok why not to say that containerList is a scope, I don't know what others think.

it would mean then containerListItem:state , containerListItem:type or containerList:itemState

cc @lstocchi that added the context for the onboarding. Thoughts ?

[deboer-tim]

+1 to using : for scopes, but I think we need to be careful where the context is a 'view' vs an 'object'. i.e. there may be cases where we want to allow adding tabs to a specific view vs where you really want to customize a container and have an action or icon in any page where that container appears. (it may still take time for us to enable it in every view, but the intention is clear)

e.g. in this case I'm assuming we'd want to allow adding an action to type container and state running, but it would also appear in the container details page at some point.

[axel7083]

@benoitf For the naming, we have to choose for the ComposeInfoUI, ContainerInfoUI, ImageInfoUI and PodInfoUI prefix to use. Any suggestation ?

[benoitf]

For the naming, we have to choose for the ComposeInfoUI, ContainerInfoUI, ImageInfoUI and PodInfoUI prefix to use. Any suggestation ?

ComposeInfoUI -> composeListItem:<property>
ContainerInfoUI -> containerListItem:<property>
ImageInfoUI -> imageListItem:<property>
PodInfoUI -> podListItem:<property>

is it what you're expecting ? based on the fact that it's displayed in a list

in details page it would be

composeDetailsItem:<property>
containerDetailsItem:<property>

[axel7083]

On a container, working fine with the prefixes.

[deboer-tim]

I really appreciate this contribution and haven't had a chance to test it yet, but so far it looks like prefixes like imageListItem are furthering one of our current problems that actions are only available in a specific place, e.g. you can push an image from the Images List, but not if you're actually looking at that image in Details. It requires every person contributing an action to know all the places where an object might appear and add to each of them. How many people will forget that pods appear in the containers list?

I know there will be cases where something should only appear in one place too, but that seems lower frequency so I'm hesitant to release this without the other way of contributing. Maybe we can do both, or do something to make it more generic or more obvious to know where to contribute?

Also, shouldn't docs/extensions/write/when-clause-context be updated? I'm ok with separate issue or PR, just don't want to lose this.

[benoitf]

I really appreciate this contribution and haven't had a chance to test it yet, but so far it looks like prefixes like imageListItem are furthering one of our current problems that actions are only available in a specific place, e.g. you can push an image from the Images List, but not if you're actually looking at that image in Details.

I think action would appear correctly in both image list and image details (or container list and container details)
as menu name that is used is dashboard/image or dashboard/container so contribution is not specifying a list or the details

It requires every person contributing an action to know all the places where an object might appear and add to each of them. How many people will forget that pods appear in the containers list?

I think person won't need to know all the places. If you need an action about pods, you will register a menu for dashboard/pod then it'll will be displayed in container list or pod details, etc.

@axel7083 last change, could you tweak the prefix as as you're sharing the same *Actions component from the ContainerList and ContainerDetails we can't have 'List' in the name of the property
so stick to containerItem, podItem, etc.

Also, shouldn't docs/extensions/write/when-clause-context be updated? I'm ok with separate issue or PR, just don't want to lose this.

@axel7083 could you open a PR against this file as well (out of this PR)

[benoitf]

Looking as like final review:

so it looks next steps (follow-up PR, etc) would be to include in containersDetails and imageDetails the inclusion of

contributedMenus = await window.getContributedMenus(MenuContext.DASHBOARD_CONTAINER);

and

contributedMenus = await window.getContributedMenus(MenuContext.DASHBOARD_IMAGE);

but it seems there is something odd here, as it looks for compose or pods we're providing the wrong contributedMenus in ContainerList

from my understanding we should have

contributedContainerMenus = await window.getContributedMenus(MenuContext.DASHBOARD_CONTAINER);
contributedPodMenus = await window.getContributedMenus(MenuContext.DASHBOARD_POD);
contributedComposeMenus = await window.getContributedMenus(MenuContext.DASHBOARD_COMPOSE);

and then for PodActions

<PodActions..

contributions="{contributedPodMenus}"

and then for ContainerActions

<ContainerActions..

contributions="{contributedContainerMenus}"

and then for ComposeActions

<ComposeActions..

contributions="{contributedComposeMenus}"

[axel7083]

Looking as like final review:

so it looks next steps (follow-up PR, etc) would be to include in containersDetails and imageDetails the inclusion of

contributedMenus = await window.getContributedMenus(MenuContext.DASHBOARD_CONTAINER);

and

contributedMenus = await window.getContributedMenus(MenuContext.DASHBOARD_IMAGE);

but it seems there is something odd here, as it looks for compose or pods we're providing the wrong contributedMenus in ContainerList

from my understanding we should have

contributedContainerMenus = await window.getContributedMenus(MenuContext.DASHBOARD_CONTAINER);
contributedPodMenus = await window.getContributedMenus(MenuContext.DASHBOARD_POD);
contributedComposeMenus = await window.getContributedMenus(MenuContext.DASHBOARD_COMPOSE);

I see, but in the curent implementation, we do not distinguish with MenuContext.DASHBOARD_CONTAINER, MenuContext.DASHBOARD_COMPOSE etc.. This is evaluated at runtime using the when condition.

We register all the menus to the ContainerList and then they are using the when condition to choose where they should be displayed.

[benoitf]

I see, but in the curent implementation, we do not distinguish with MenuContext.DASHBOARD_CONTAINER, MenuContext.DASHBOARD_COMPOSE etc.. This is evaluated at runtime using the when condition.

We register all the menus to the ContainerList and then they are using the when condition to choose where they should be displayed.

yes but this is not the expected behavior

b/c you register a command to be displayed for a condition for different purpose, like for an image, for a pod or for a compose object.

then, in your when clause, you'll use containerItem or composeItem

when a fine grained object, but default is that it's added.

so if you say, I want to bind an action for a CONTAINER, you don't expect to see the action for a POD or for a Compose object

[axel7083]

I see, but in the curent implementation, we do not distinguish with MenuContext.DASHBOARD_CONTAINER, MenuContext.DASHBOARD_COMPOSE etc.. This is evaluated at runtime using the when condition.

We register all the menus to the ContainerList and then they are using the when condition to choose where they should be displayed.

yes but this is not the expected behavior

b/c you register a command to be displayed for a condition for different purpose, like for an image, for a pod or for a compose object.

then, in your when clause, you'll use containerItem or composeItem

when a fine grained object, but default is that it's added.

so if you say, I want to bind an action for a CONTAINER, you don't expect to see the action for a POD or for a Compose object

The following change is more relevant to the previous PR about adding the DASHBOARD_CONTAINER than the when condition, should I include the change in this PR ?

The change would include

• adding await window.getContributedMenus(MenuContext.DASHBOARD_POD); to the <PodActionss>
• adding await window.getContributedMenus(MenuContext.DASHBOARD_COMPOSE); to the <ComposeActions>
• adding await window.getContributedMenus(MenuContext.DASHBOARD_CONTAINER); to the <ContainerActions>
• adding await window.getContributedMenus(MenuContext.DASHBOARD_IMAGE); to the <ImageActions>
• deleting the await window.getContributedMenus from ListImage and ListContainer

[benoitf]

I'm fine with your proposal

[benoitf]

and if it helps, you can merge some commits together and separate changes with different commits having a meaningful set of changes

[axel7083]

I'm fine with your proposal

Perfect then

and if it helps, you can merge some commits together and separate changes with different commits having a meaningful set of changes

I will do everything here

[benoitf]

it's not related to this PR but I think we should have contributedMenus being in a store as new items/deletion of items should refresh the UI on the fly

#4039

[benoitf]

@axel7083 we still have unit tests failing

I'll probably merge on monday as never merge big chunks on friday :-)