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

Initial usage of the typescript-sdk #4762

Closed
wants to merge 7 commits into from

Conversation

andreaTP
Copy link
Member

Based on #4338

I updated everything on the Kiota side and converted one simple endpoint to use the TS SDK in the UI.

  • should we move everything to Kiota?
  • should we do it in a single PR? (there are 45 endpoints, it's doable but not nice to review)
  • which models should we use as a source of truth?
  • what do you think about the approach for authentication?

@apicurio-bot
Copy link

apicurio-bot bot commented Jun 11, 2024

Thank you for creating a pull request!

Pinging @EricWittmann to respond or triage.

@apicurio-bot apicurio-bot bot added the area/ui label Jun 11, 2024
@EricWittmann
Copy link
Member

EricWittmann commented Jun 11, 2024

* should we move everything to Kiota?

Yes.

* should we do it in a single PR? (there are 45 endpoints, it's doable but not nice to review)

Let's get this PR merged first, then I'll convert everything else in a single followup PR.

* which `model`s should we use as a source of truth?

I'll delete all the existing models in favor of the Kiota generated ones.

* what do you think about the approach for authentication?

LGTM, although it's a bit hard to review rest.utils.ts code check warnings/errors. LOL what editor are you using that is still using TAB characters?!

@andreaTP
Copy link
Member Author

LOL what editor are you using that is still using TAB characters?!

I told you I'm bad at formatting code 😛 it's probably copy pasted.
I get this PR in a final shape and we can convert the remaining endpoints.
Warning: we "might" need some tweaks for files upload/download but let see ...

@@ -20,5 +20,8 @@
"devDependencies": {
"rimraf": "5.0.7",
"shelljs": "0.8.5"
},
"dependencies": {
"@apicurio/apicurio-registry-client": "file:../typescript-sdk"
Copy link
Member Author

Choose a reason for hiding this comment

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

this uses npm link ... not sure if we can improve it anyhow ...

Copy link
Member

Choose a reason for hiding this comment

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

ok - i'll see what I can do on this



const AXIOS = axios.create();
Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally, at the end of the process, the rest of this file should go away.


export function createAuthProvider(auth: AuthService): AuthenticationProvider {
if (auth.isOidcAuthEnabled()) {
return new TokenAuthenticationProvider("Bearer", async () => auth.getToken().then(v => v!));
Copy link
Member Author

Choose a reason for hiding this comment

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

Just to be sure, are we testing those configurations of the UI in CI?

Copy link
Member

Choose a reason for hiding this comment

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

What configurations? We're only doing basic testing of the UI in CI - in two places. We have UI tests in the registry repo, and we have some in the 3scale dev environment as well.

More UI testing is for sure needed - currents tests are mostly smoke tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we test the UI in an environment where OIDC is enables and Keycloak configured?

Copy link
Member

Choose a reason for hiding this comment

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

In the 3scale dev environment, yes. But not in registry CI.

@andreaTP andreaTP marked this pull request as ready for review June 12, 2024 16:04
@andreaTP andreaTP changed the title [WIP] Initial usage of the typescript-sdk Initial usage of the typescript-sdk Jun 12, 2024
@andreaTP
Copy link
Member Author

I hope I have addressed all the comments, a couple of additional questions 🙂 :

  • are we convinced about committing the generated code?
  • is it ok to rebuild the ts-sdk in CI all the time?

@EricWittmann EricWittmann self-assigned this Jun 14, 2024
@EricWittmann
Copy link
Member

EricWittmann commented Jun 20, 2024

I hope I have addressed all the comments, a couple of additional questions 🙂 :

* are we convinced about committing the generated code?

I'm just getting to this now, but for sure we do not want to commit the generated code. And this PR doesn't seem to do that...which is good.

* is it ok to rebuild the `ts-sdk `in CI all the time?

We could optimize I guess - only generate if the openapi.json file changes. But I don't think we're doing that for other SDKs, are we?

@andreaTP
Copy link
Member Author

And this PR doesn't seem to do that...which is good.

I was mistaken due to the LOC changed 😅 .

We could optimize I guess - only generate if the openapi.json file changes. But I don't think we're doing that for other SDKs, are we?

No, I think it's fine with this proposed setup.

@EricWittmann
Copy link
Member

Closing in favor of #4806

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.

2 participants