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

POC: Generate RTK Query clients for App Platform APIs #87545

Closed

Conversation

joshhunt
Copy link
Contributor

@joshhunt joshhunt commented May 9, 2024

Part of #87540

@dprokop
Copy link
Member

dprokop commented May 9, 2024

@bfmatei @ryantxu may be related to the work you did with #83339 and #85266

@ryantxu
Copy link
Member

ryantxu commented May 9, 2024

interesting to see what comes out of codegen... honestly kinda gross and mostly highlights all the things we do not want people using from the UI 😬

The only part that is not boilerplate (exactly identical for any ResourceServer) is the typed response.
https://github.com/grafana/grafana/pull/87545/files#diff-74572fff270cec266440888d7a0f2e6ac7616fd5f8b6c4ca38d3f36f74974e2bR423

The openapi spec just identifies a bunch of our key metadata as [key: string]: string;
https://github.com/grafana/grafana/pull/87545/files#diff-9e23f389ab3783388429febc1dd1aa39265aa73280e87c555a2fdb310d153720R1768. It would be nice to have this better defined like in: https://github.com/grafana/grafana/blob/main/public/app/features/apiserver/types.ts#L44

@ryantxu
Copy link
Member

ryantxu commented May 9, 2024

I have not yet explored how https://github.com/kubernetes/dashboard/tree/master/modules/web handles their client -- it also supports the "watch" command so that may be interesting to look at

@joshhunt joshhunt force-pushed the joshhunt/generate-k8s-rtk-q-apis branch from 851bc48 to f3c75ae Compare May 9, 2024 14:30
@joshhunt joshhunt force-pushed the joshhunt/generate-k8s-rtk-q-apis branch from d2acda2 to 0e2a38c Compare May 10, 2024 11:34
getApiResources: build.query<GetApiResourcesApiResponse, GetApiResourcesApiArg>({
query: () => ({ url: `/apis/playlist.grafana.app/v0alpha1/` }),
}),
listPlaylists: build.query<ListPlaylistsApiResponse, ListPlaylistsApiArg>({
Copy link
Contributor

Choose a reason for hiding this comment

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

How did you get these endpoint names (without the namespace stuff), manual search + replace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah - I manually edited the OpenAPI spec and then generated from that 0e2a38c

@@ -3,6 +3,17 @@
"info": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we expect each resource to contain these spec files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! They basically do already - see all the zz_generated.openapi.go files in the repo. This is just a JSON serialisation of those files.

}
}
}
],
Copy link
Contributor

Choose a reason for hiding this comment

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

So this abstracts the base URL, so we don't need to define those separately for each endpoint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's the role of it in OpenAPI specs.

But, here I've used this to avoid RTK Query generating URLs that require a namespace parameter, and it'll be defined centrally/manually in the baseAPI.ts file. It's half hack, half what it's designed for.

@@ -23,6 +23,25 @@ const config: ConfigFile = {
'getDashboardByUid',
],
},

Copy link
Contributor

Choose a reason for hiding this comment

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

Wonder how to make this work for Enterprise. Should we define the same file there with own paths and then somehow merge it with this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's a valid question. Where would the API clients for enterprise live? Given the legacy api enterprise spec lives in the OSS repo, i think it's fine to generate the clients into OSS repo as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the generated clients should be in the same place as the feature (the FE part), under the api folder, so Enterprise clients would be in the Enterprise repo. As for the scripts that generate those clients, I'm not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's valid. The problem is that there's this desire to share at least some of these API clients with plugins, probably through the grafana-runtime package.

baseURL: `/apis/playlist.grafana.app/v0alpha1/namespaces/${config.namespace}/`,
}),
endpoints: () => ({}),
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This file looks like it'll be quite similar for all clients, could it be also generated?

@joshhunt
Copy link
Contributor Author

There's probably not going to be much more work on this branch for now, so I'll close the PR and see what happens next :).

Comments are still welcome!

@joshhunt joshhunt closed this May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants