Skip to content

add graphql endpoints to match the REST logic#61

Closed
stan-dot wants to merge 3 commits intoDiamondLightSource:mainfrom
stan-dot:graphql-iter-1
Closed

add graphql endpoints to match the REST logic#61
stan-dot wants to merge 3 commits intoDiamondLightSource:mainfrom
stan-dot:graphql-iter-1

Conversation

@stan-dot
Copy link

Initial implementation towards DiamondLightSource/sm-bluesky#14

@Relm-Arrowny

@stan-dot
Copy link
Author

@callumforrester the core team has people familiar with setting a graphql that works with the supergraph, I'd welcome your input

Copy link

@callumforrester callumforrester left a comment

Choose a reason for hiding this comment

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

Without understanding the use cases I don't know if this is the right API, but as you say it's iter 1 (although please use a more descriptive PR title). I suggest someone from MX does the full review here.

Co-authored-by: Callum Forrester <callum.forrester96@gmail.com>
@stan-dot stan-dot changed the title Graphql iter 1 addding graphql endpoints to match the REST logic Mar 25, 2025
@stan-dot
Copy link
Author

@GDYendell I hear that you're the person to ask about the arcane details of supergraph integration, how to do this?

@callumforrester callumforrester changed the title addding graphql endpoints to match the REST logic add graphql endpoints to match the REST logic Mar 25, 2025
@GDYendell
Copy link

GDYendell commented Mar 26, 2025

There are some docs on this here and you can look at https://github.com/DiamondLightSource/workflows as an example project that is already doing this.

@stan-dot
Copy link
Author

@DominicOram I'll look into the supergraph docs to add it - there will be data export here as a CI job and a PR to the graph-federation repo.

in the meantime, is the implementation directionally correct?

@olliesilvester
Copy link
Collaborator

olliesilvester commented Mar 27, 2025

I'll look into the supergraph docs to add it - there will be data export here as a CI job and a PR to the graph-federation repo.

I wasn't sure if this actually needs to be exposed to the supergraph, but I guess it depends on what applications (other than BlueAPI) will be communicating with it. If it's only BlueAPI then I imagine we don't need to integrate it

@callumforrester
Copy link

I might suggest letting the API evolve a bit before exposing it, right now it is basically just a key/value store

@stan-dot
Copy link
Author

@olliesilvester okay so no supergraph needed, then I'm moving on to work on making this ready for review

@olliesilvester
Copy link
Collaborator

Discussed with @stan-dot , closing this as stale since the config server is going in a different direction now

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.

4 participants