Skip to content
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

feat: add api to register cli updater (#4690) #5064

Merged
merged 1 commit into from Dec 1, 2023

Conversation

lstocchi
Copy link
Contributor

What does this PR do?

It adds the logic to register an cli updater which will be called on request to update a cli.

Screenshot/screencast of this PR

For gif to see how it will be used/work check #5054

What issues does this PR fix or reference?

this is a part of #4690.

How to test this PR?

  1. run tests

Signed-off-by: lstocchi <lstocchi@redhat.com>
@lstocchi lstocchi requested review from benoitf and a team as code owners November 30, 2023 12:32
@lstocchi lstocchi requested review from dgolovin, cdrage and jeffmaury and removed request for a team November 30, 2023 12:32
@@ -2506,6 +2522,12 @@ declare module '@podman-desktop/api' {
id: string;
label: string;
};

updateVersion(version: CliToolUpdateOptions): void;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hello, I'm wondering why we need to pass so many things in the update function like displayName, images, path, etc ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for me a new version could have a different description, display name, icon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe in a new version there is something super which is supported or not supported anymore and you want to mention it in the description, or mark it in the name, or the icon has different color. Idk

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in that case, maybe the extension unregister the current CliTool and register a new one ?
for the images, I would say that as they come with the extension, probably if CLI has a new icon we will update the new icon in a newest extension but it won't be different in all the calls (like between register and update)

I'll wait other reviews, but it looks a little bit over-engineering to me.

Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

displayName?: string;
markdownDescription?: string;
images?: ProviderImages;
path?: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also include the asset name, as when retrieving the / installing the new update, you'll have to pass in the asset name determined by the extension (example, hey, install arm64 darwin).

Or should we just do quickpick / prompt for now?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's the extension that is doing the update so extension is aware on asset name to pick-up

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the extension has already a reference of the cli that it wants to update as it registered it during the activation. So you're going to do cli.update(new data) from the extension when the user asks for it. There is no need for the asset name.
I didn't make it updatable as I see it like an id. You can change the display name, description, version but the asset name should be the same, otherwise you're most probably registering a new cli tool.

Copy link
Contributor

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lstocchi lstocchi requested a review from cdrage November 30, 2023 16:42
@cdrage cdrage merged commit 019f1b6 into containers:main Dec 1, 2023
9 checks passed
@podman-desktop-bot podman-desktop-bot added this to the 1.6.0 milestone Dec 1, 2023
@lstocchi lstocchi deleted the i4690_1 branch December 1, 2023 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants