strimzi-ui icon indicating copy to clipboard operation
strimzi-ui copied to clipboard

feat: add graphql cli

Open rkpattnaik780 opened this issue 4 years ago • 20 comments

Addresses: #37

Signed-off-by: Ramakrishna Pattnaik [email protected]

rkpattnaik780 avatar Nov 25 '20 10:11 rkpattnaik780

PR Report

Bundle Sizes

View bundle sizes for 'client'
Bundle New Size Original Size Increase/Decrease File
1.bundle.js 178.72KB 631.33KB -72% 1.bundle.js
Strimzi UI JS Bundle 3.64KB 11.08KB -67% main.bundle.js
Overall bundle size change: -71.61%
View bundle sizes for 'server'
Bundle New Size Original Size Increase/Decrease File
Strimzi UI Server JS Bundle 18.17KB 26.67KB -32% main.js
Overall bundle size change: -31.87%

Test Coverage

View test coverage
File Lines Statement Functions Branches
Total 100% 100% 100% 100%
client/Bootstrap/Navigation/useRouteConfig/useRouteConfig.hook.ts 100% 100% 100% 100%
client/Contexts/ConfigFeatureFlag/Context.tsx 100% 100% 100% 100%
client/Panels/Home/Home.tsx 100% 100% 100% 100%
File Lines Statement Functions Branches
Total 100% 100% 100% 100%
server/api/controller.ts 100% 100% 100% 100%
server/api/router.ts 100% 100% 100% 100%
server/client/controller.ts 100% 100% 100% 100%
server/client/router.ts 100% 100% 100% 100%
server/config/controller.ts 100% 100% 100% 100%
server/config/router.ts 100% 100% 100% 100%
server/core/app.ts 100% 100% 100% 100%
server/core/modules.ts 100% 100% 100% 100%
server/log/router.ts 100% 100% 100% 100%
server/mockapi/data.ts 100% 100% 100% 100%
server/mockapi/router.ts 100% 100% 100% 100%

Triggered by commit: 4a5d063b0a894541eed60c2de716e17467db5007

github-actions[bot] avatar Nov 25 '20 10:11 github-actions[bot]

To generate the entity model, two libraries were explored: graphql-cli and graphql-code-generator. It was decided to go with graphql-cli as it offered more options over graphql-code-generator.

The various ways to load schema have been discussed below:

  1. Load from a URL http://localhost:3000/graphql
  2. Load from a local file src/**/*.graphql

These approaches would have been fine for development bit could cause inconsistencies in case strimzi-admin schema gets changed/modified.

  1. Git sub-modules

Managing github modules can be cumbersome.

  1. Using github loader and pointing to a tag with the desired schema.

Github loader is able to download schema associated with the tag but it needs a Github personal access token which has to be passed as an environment variable. Furthermore it seems unnecessary to use token to get files from public repo.

  1. Providing the URL to raw schema file https://raw.githubusercontent.com/rkpattnaik780/enmasse/14c4e82a0c13370d5e14a74669982e1a1105a0ba/console/console-init/ui/mock-console-server/schema.graphql

This gets the work done for now, it requires to pass the commit ID of the tag. However we might need to fallback to a traditional method as pointing to a different github repo does not count as best practice.

rkpattnaik780 avatar Nov 25 '20 10:11 rkpattnaik780

Amazing work. From my side to move this next step further it will be good to bring the actual graphql endpoint and see if we can generate hooks.

wtrocki avatar Nov 25 '20 11:11 wtrocki

Yes. Schema first development in the glance. Codegen generates types - Makes server side resolvers fully typed and Client side types and even data layer. Practically sealing entire API contract.

The best pattern for working is to snapshot server-side schema in the repo to not get exposed by "Friday Evening PR merge" problems etc. - Getting schema updates from the server when requested/released rather than using github master

wtrocki avatar Nov 25 '20 17:11 wtrocki

This schema can then be used/referenced in our MockApi too (in effect completing #50 )

GraphQL CLI can create mock out of the box.

wtrocki avatar Nov 25 '20 17:11 wtrocki

Sorry for the delay in following up @rkpattnaik780 / @wtrocki - thanks for answering my first set of questions.

As is today, we have code which will introspect the backend to figure out what the schema contains (at runtime). We also will have code which will discover what the user has authorization to do. As described in the docs, the intent of the entity model was to combine these two inputs into a 'yes'/'no' CRUD abstraction (+use case specific items, such as live updates (subscriptions) etc).

What I am missing here (and please correct me/let me know the intent) is how a generated version of the schema will help provide this model. We introspect the server already, so we know what is there. We (will have) information about what the user can do - how were you seeing these pieces come together?

matthew-chirgwin avatar Dec 03 '20 14:12 matthew-chirgwin

As is today, we have code which will introspect the backend to figure out what the schema contains

Sounds complex but I think I understand what intention is.

What I am missing here (and please correct me/let me know the intent) is how a generated version of the schema will help provide this model.

Little bit confused. We do not generate schema - more like typings (if we would want we can generate client side queries but this will be overkill for such simple schema) In my mind generation is not runtime artifact, but development artifact and it is not affecting any runtime architecture mentioned above. Feel free to ping me on slack to clarify. I think we might be just circling around different levels of software dev process but our understanding of those processes is right.

We introspect the server already, so we know what is there. We (will have) information about what the user can do - how were you seeing these pieces come together?

I had some chat with folks about where schema should land etc. Introspecting server has multiple drawbacks of snapshoting schema (which is being used very often - especially with schemas that have more than >100 types.

wtrocki avatar Dec 03 '20 15:12 wtrocki

@wtrocki @matthew-chirgwin - my reading of this PR is that it's creating TS types from a graphql schema for development purposes. So that we can nicely handle gql results in TS.

While this is useful from a development perspective, I believe the intent of the entity model is instead a way to model the available capabilities in the system, based off the result of a graphql introspection.

E.g - introspect shows there is a topics query - and user introspection shows the user can create topics. As a result - the entity model (which is passed around the UI) must be built at runtime to state that we can render a "create topic" page.

nictownsend avatar Dec 03 '20 16:12 nictownsend

I'm not sure how you can create types at runtime in a strongly typed language.

pmuir avatar Dec 03 '20 16:12 pmuir

my reading of this PR is that it's creating TS types from a graphql schema for development purposes. So that we can nicely handle gql results in TS.

It is much more. Defacto standard for GraphQL clients really (More generic one to the Relay) . It creates an entire data access layer using client requirements specification (queries, mutations). It means that there is no need to deal with graphql in the code at all and the code is actually detached. It keeps code detached from actual client (you can swap from Apollo to URQL in 20 seconds) etc. Most of the GraphQL foundation members use this approach (either thru Apollo CodeGen or GraphQL Code Generator)

While this is useful from a development perspective, I believe the intent of the entity model is instead a way to model the available capabilities in the system, based off the result of a graphql introspection.

I understand what we are saying, but it is not clear what requirement pushed us to have that architecture and also do not see why architecture is blocking what we want to do here. From my experience it looks like we are using introspection in production which has some drawbacks, but still if there is reason to do so.. why not.. I do not want to change the discussion from: "do we need codegen" to "is this architecture valid" because I'm not officially involved in the project, but in my understanding, there is no real blocker for having this PR merged.

wtrocki avatar Dec 03 '20 16:12 wtrocki

Major preface: I do not want to stifle the discussion, or options required to provide the entity model in any way. If this is part of an approach to achieve what is requested in #37 then I am all for understanding/discussing it.

It sounds to me like there are some very crossed wires here. From your description @wtrocki it appears that this tooling will in effect replace/supersede our usage of Apollo in the client side by generating code to interact/access the backend based on a referenced schema.

The reason we went with introspection at runtime is we want to use the available schema as a mechanism (in combination with other sources of data) to enable and disable features. That is the intent of #37 - a model to combine results from introspection, authorisation etc, to provide a simple Yes/No answer to if a type, field or operation are possible for the UI to use were needed. Eg, if no ‘Topic’ type exists on the backend, we should not show any topic pages/data/UI etc.

I am curious as well, can this code generation approach deal with things like schema federation? Again, a reason we went with a runtime check here is to allow flexibility. Strimzi-admin today will host a GQL api which will service just topic information - but this could be (optionally) extended in due course, and I expect modules (say for metrics) would be federated into the overall schema if there was ‘metrics’ enabled/deployed as a part of a Strimzi deployment.

matthew-chirgwin avatar Dec 04 '20 09:12 matthew-chirgwin

My understanding of this issue is to generate an entity model. This is essentially a set of typescript types/interfaces that represent the schema. This cannot be generated at runtime, we have to code against it. This is somewhat orthogonal to introspection which is to determine if the capability exists at runtime.

Or were you thinking to go with a detyped entity model?

I would avoid schema federation unless it's critical for the functional requirements. It is complex, causes some things to not work well, and will slow the UI down.

pmuir avatar Dec 04 '20 12:12 pmuir

While this is useful from a development perspective, I believe the intent of the entity model is instead a way to model the available capabilities in the system, based off the result of a graphql introspection.

The introspection is already done. This should be combined with the entity model to determine what can be rendered.

pmuir avatar Dec 04 '20 12:12 pmuir

E.g - introspect shows there is a topics query - and user introspection shows the user can create topics. As a result - the entity model (which is passed around the UI) must be built at runtime to state that we can render a "create topic" page.

pmuir avatar Dec 04 '20 12:12 pmuir

My understanding of this issue is to generate an entity model. This is essentially a set of typescript types/interfaces that represent the schema. This cannot be generated at runtime, we have to code against it. This is somewhat orthogonal to introspection which is to determine if the capability exists at runtime.

Agreed. For sake of the discussion/clarity, the original ask of the issue is to write a piece of code (at develop time), called at runtime which in effect reduces the result of introspection, authorization etc to an entity (and operations on that entity) centric view for the UI to make use of. Using the example of the topics page, if the introspection result shows no mutation to create, no 'create topic' button would be shown. Similarly, if the mutation was there, but the user doesnt have the authoization to create topics, again, the button would not be shown.

Having said that, there are of course other ways of achieving the same effect. What I fail to see here is how what this PR contains is/would be used to achieve the outcome of this issue.

matthew-chirgwin avatar Dec 04 '20 13:12 matthew-chirgwin

The reason we went with introspection at runtime is we want to use the available schema as a mechanism (in combination with other sources of data) to enable and disable features.

There are much better (and simpler mechanisms to achieve that with GraphQL). Using introspection for this purpose has many drawbacks. Generally, Strimzi UI is an open source project so the simpler architecture it has the more contributors will attract in the future. We can look into something lean, I could recommend to see if we and start snapshotting schema (introspection can still be used). This is not a big architecture change. However feature enablement might be done better with

  • metadata (https://github.com/aerogear/graphql-metadata)
  • Directives
  • GraphQL extensions fields
  • Meta tags

Introspection advisably would be development artifact. Happy to discuss.

Generally, the more we diverge from how graphql is normally being used in the big players and move from standards the less possibilities we will have in the future to mitigate problems like performance etc. For example: https://www.apollographql.com/docs/apollo-server/performance/apq/

Strimzi-admin today will host a GQL api which will service just topic information - but this could be (optionally) extended in due course, and I expect modules (say for metrics) would be federated into the overall schema if there was ‘metrics’ enabled/deployed as a part of a Strimzi deployment.

Ok. Then it would be good to put some public proposal for this and look for possible options.

I am curious as well, can this code generation approach deal with things like schema federation?

Schema federation for me will be different topic Based on my understanding Node.js gateway needs to be more router than federation gateway as all servers schema will be the same (with some subsets of features/enablement etc.)

In Red Hat we have our own high-performance GraphQL Federation solution: https://github.com/chirino/graphql-gw

We also had deployed Apollo Federation etc.

wtrocki avatar Dec 04 '20 15:12 wtrocki

+100 to what @pmuir was saying. To be clear I'm here as a community member not as Red Hatter. Since this is a public repository in high profile project it would be better to bring this to more top level discussion somewhere on proposal/issue where we could discuss with the context of the change.

I would like to be able to point out my colleague who written introspection standard for GraphQL to some thread without much noise etc. so he can give us independent view. My take can be opiniated as I have been writing things certain way and build some Node libs etc. Once we get clarification on the path we can really make Strimzi UI successful and popular open source project.

wtrocki avatar Dec 04 '20 16:12 wtrocki

I've re-read the original description of this - https://github.com/strimzi/strimzi-ui/blob/master/docs/Architecture.md#entity-model - and I think some of confusion may be that what the architecture calls a "entity model" is typically called a data access layer (e.g. https://en.wikipedia.org/wiki/Data_access_object) or a repository, whilst typically the "entity model" refers to entities and their relationships (e.g. https://en.wikipedia.org/wiki/Entity%E2%80%93relationship_model).

pmuir avatar Dec 07 '20 20:12 pmuir

Leaving to one side the debate about whether the data access is cocrrect.

How are we specifying the entities (e.g. Topic) and their fields (e.g. partitions)? These types need to be available at development time.

pmuir avatar Dec 07 '20 20:12 pmuir

Given the discussion in the PR and the original intent, I believe the next steps are as follows:

  • We use the tooling in this PR to generate a set of files containing all the possible types/fields from the strimzi-admin schema
    • Follow on question: as these are used in the client, should the go into a folder in the client directory (as they also need to be checked in)
  • The existing documentation is updated (as a part of this PR) to mention this occurs, and that said generated code is used in a ‘reducer like’ process with other data (the results of introspection, authentication, authorisation etc) to output a view the rest of the UI can use to enable/disable features based on what is available at run time
  • Change/rename ‘Entity model’ to something more explicit to this process (to avoid future confusion).

matthew-chirgwin avatar Dec 15 '20 16:12 matthew-chirgwin