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

Kiota powered, in browser, client generation #3082

Merged
merged 10 commits into from
Mar 22, 2023

Conversation

andreaTP
Copy link
Member

@andreaTP andreaTP commented Jan 13, 2023

Those changes are completely in the UI, you can check them out using this command:

docker run --rm -it -p 8080:8080 docker.io/andreatp/apicurio-registry-kiota:latest

The Kiota compilation to Wasm and the production of the relevant folder happens in this repository:
https://github.com/andreaTP/apicurio-client-gen-poc

we might want to make it more official if we want to fully support it.

@apicurio-bot
Copy link

apicurio-bot bot commented Jan 13, 2023

Thank you for creating a pull request!

Pinging @EricWittmann to respond or triage.

<Form>
<Grid hasGutter md={6}>
<GridItem span={12}>
<Label>[EXPERIMENTAL] This is "in-browser" generation of the client code using&nbsp;<a href="https://github.com/microsoft/kiota">Kiota</a>, please refer to&nbsp;
Copy link

Choose a reason for hiding this comment

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

❤️

onSelect={this.onLanguageSelect}
isOpen={this.state.languageIsExpanded}
dropdownItems={[
<DropdownItem id="Java" key="Java" data-testid="form-type-auto"><i>Java</i></DropdownItem>,
Copy link

Choose a reason for hiding this comment

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

may I suggest to sort them alphabetically? Also Kiota has a maturity matrix for languages, you might want to feature that too.

Copy link
Member Author

Choose a reason for hiding this comment

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

done 👍

label="Client Class Name"
fieldId="form-client-name"
>
<TextInput
Copy link

Choose a reason for hiding this comment

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

I do think you should feature the include and exclude filters, or have some kind of endpoint selection mechanism. A big value proposition of Kiota is being able to generate specifically for the API surface the client cares about. Another aspect to that is that often some descriptions won't be the cleanest, and filtering out things that are failing is a quick way to get around and get things done.

Copy link
Member Author

Choose a reason for hiding this comment

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

done 👍

};
}

private doGenerate = async (): Promise<void> => {
Copy link

Choose a reason for hiding this comment

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

from a UX perspective, having some kind of indication something is happening in the background would be helpful I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

agree, added a spinner 👍

@baywet
Copy link

baywet commented Jan 13, 2023

here is a short screencap of the feature for those interested.

apicurio-integration

@andreaTP
Copy link
Member Author

andreaTP commented Jan 16, 2023

@baywet thanks for the review!
I hope I have addressed all your points, and I think it looks a bit nicer now 🙂

Also updated the Container image.

@andreaTP
Copy link
Member Author

This would also need documentation @smccarthy-ie 🙏

@andreaTP
Copy link
Member Author

Now available in the Operate First environment:
https://apicurio-registry-mt-ui-mt-apicurio-apicurio-registry.apps.smaug.na.operate-first.cloud/

@baywet
Copy link

baywet commented Jan 16, 2023

@andreaTP now that Kiota is (almost) integrated with apicurio, I'm wondering whether the search command from kiota should be able to sign-in and search an apicurio instance to go full circle?

@andreaTP
Copy link
Member Author

I'm wondering whether the search command from kiota should be able to sign-in and search an apicurio instance to go full circle?

@baywet interesting idea! But I'm not 100% sure about how the integration can work here, a few questions:

  • should the integration work only through our managed service or also on other instances?
  • how do you see the Kiota Search functionality working in an integrated environment?

A slightly diverging angle in the 2 projects is that Kiota concentrates on OpenAPI while here Registry does support a number of different formats, do you have any plan for supporting other/more formats or should the feature be limited only to OpenAPI?

@baywet
Copy link

baywet commented Jan 16, 2023

started a new discussion here to avoid side-tracking this PR.

@andreaTP andreaTP changed the title [POC] Kiota powered, in browser, client generation Kiota powered, in browser, client generation Jan 17, 2023
@EricWittmann
Copy link
Member

We should decide how to get this merged. It needs to be behind a feature flag at least.

@andreaTP
Copy link
Member Author

andreaTP commented Feb 1, 2023

It needs to be behind a feature flag at least.

Do you wanna "hide" the feature or be able to completely get rid of the integration?

@EricWittmann
Copy link
Member

Just hiding it is fine I think. Hidden by default (for now), with a config variable to enable it. Then we can set the config variable to enabled in Operate First.

@andreaTP andreaTP force-pushed the poc-client-generation branch from ccb8c8c to 930b046 Compare February 15, 2023 13:01
@andreaTP
Copy link
Member Author

Sorry for the delayed feedback, the button is now behind a flag here:
930b046

@carlesarnal
Copy link
Member

This is now green. Unless there are any objections, I'll merge this mid-next week.

@andreaTP
Copy link
Member Author

YaY!

@EricWittmann
Copy link
Member

LGTM ready to merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants