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

Create/Delete graph variants using Rover CLI #722

Closed
AndreyIlyunin opened this issue Aug 10, 2021 · 14 comments · Fixed by #861
Closed

Create/Delete graph variants using Rover CLI #722

AndreyIlyunin opened this issue Aug 10, 2021 · 14 comments · Fixed by #861
Labels
feature 🎉 new commands, flags, functionality, and improved error messages
Milestone

Comments

@AndreyIlyunin
Copy link

Creating and deleting graph variants using Rover CLI

Ability to manage graph variants using Rover CLI:

  • create new graph variants by providing its names/IDs*
  • delete graph variants by their names/IDs*

* It is necessary to have such an opportunity, as it launched from CI/CD pipeline and all we have here is a thing like a branch name, and no storage ability to remember generated identifiers.


This will helps users to move to the managed schemas totally.
And the ability to use Rover CLI subgraph publish features for any environment in the same manner.

For example, the use-case of mine:
My team generating a staging environment per feature.
Our flow is: create a new branch, code it, create new staging env using this branch (that is production-like, with a self subdomain, DBs, and services), merge to master, destroy staging env. (I believe it is not a unique flow)

Our production is using managed schema features,
but we are required to use serviceList and experimental_pollInterval just because there is no way to create and destroy graph variants via CI for dynamically created staging envs.
Currently it is only available through the UI, but this adds to much routines and mistakes-leaky as for humans nature:


So we need to have this in Rover CLI: create a new variant by name, publish its subgraphs (this one is already here, thnx), delete this variant when it comes obsolete.

Please let me know what do you think about it!
Thanks.

@AndreyIlyunin AndreyIlyunin added feature 🎉 new commands, flags, functionality, and improved error messages triage issues and PRs that need to be triaged labels Aug 10, 2021
@EverlastingBugstopper
Copy link
Contributor

EverlastingBugstopper commented Aug 18, 2021

Hey @AndreyIlyunin - we're doing some thinking around graph provisioning w/Rover as it's something we've wanted internally as well (see #453)

I've created some preliminary flowcharts for two possible commands rover graph create and rover graph delete that primarily operate on creating and deleting individual variants.

The two flowcharts are here and should be public, if you have any feedback on them (good or bad!) please do let me know 😄

  1. rover graph create
  2. rover graph delete

we imagine the flow in CI looking very similar to what you have proposed here:

Our flow is: create a new branch, code it, create new staging env using this branch (that is production-like, with a self subdomain, DBs, and services), merge to master, destroy staging env. (I believe it is not a unique flow)

  1. Create new branch
  2. Make changes to GraphQL Schema
  3. Create new staging env (in workflow, this would run something like rover graph create mygraph@my-feature-branch-a021dz --account-id my-account in addition to a loop through rover subgraph publish)
  4. Merge to main
  5. Destroy staging env (in workflow, rover graph delete mygraph@my-feature-branch-a021dz)

@prasek
Copy link

prasek commented Aug 18, 2021

@AndreyIlyunin if this is a common use case, one possible optimization re: (3) above:

Create new staging env (in workflow, this would run something like rover graph create mygraph@my-feature-branch-a021dz --account-id my-account in addition to a loop through rover subgraph publish)

what about something like this instead?
rover supergraph publish my-graph@ci --config supergraph.yaml which took a config yaml similar to rover supergraph compose

subgraphs:
  films:
    routing_url: https://films.example.com
    schema: 
      file: ./films.graphql
  people:
    routing_url: https://people.example.com
    schema: 
      file: ./people.graphql

this would also clear up confusion around having to the use --convert option in rover subgraph publish to get a federated graph, as rover supergraph publish would implicitly plass the --convert option behind the scenes.

also we could probably add an option to --ensureNewGraphRef that would create a new variant if it didn't already exist and recreate the variant if it did exist (to ensure a fresh starting point for tests to run), so you could just run a single command in your CI jobs:

rover supergraph publish my-graph@ci --config supergraph.yaml --ensureNewGraphRef

@AndreyIlyunin
Copy link
Author

Hi @EverlastingBugstopper

  1. Create new branch
  2. Make changes to GraphQL Schema
  3. Create new staging env (in workflow, this would run something like rover graph create mygraph@my-feature-branch-a021dz --account-id my-account in addition to a loop through rover subgraph publish)
  4. Merge to main
  5. Destroy staging env (in workflow, rover graph delete mygraph@my-feature-branch-a021dz)

Actually, this is what we are looking for. Thanks!

Just to clarify, as it seems a bit tricky from the comments around the new variant creation flow,
If this is the way you are going to implement this:

$ rover create graph@staging-1 --account-id my-account
$ rover subgraph publish graph@staging-1 --schema.graphql

it will work for us.

What about $ rover graph delete graph@staging-1 --force - it is just required to keep the studio clean, without stale variants that contain the "404" URLs. So this one clearly hits the target.


And I've mentioned that you propose to extend the rover graph publish command to have the "find or create" by name and publish operations behind a single command.
This one will work for us even better, as we don't actually care if this graph@staging-1 exists from the deployment perspective.
It is much handy to have a single command for the initial deployment (that should create variant and publish subgraph) and for future deployments (that should update the existing one).
Just include the --force flag for the CI robots too :)

Anyway, we need an option to handle this programmatically, so both options are great.
Will waiting for the future versions of Rover. Thanks 👍🏻

@AndreyIlyunin
Copy link
Author

also we could probably add an option to --ensureNewGraphRef that would create a new variant if it didn't already exist and recreate the variant if it did exist (to ensure a fresh starting point for tests to run), so you could just run a single command in your CI jobs:

rover supergraph publish my-graph@ci --config supergraph.yaml --ensureNewGraphRef

Yep. The single command is the most attractive format for working with variants from the CI.
Not sure about the configs with the list, as it looks just like the same replacement for the serviceList, but if this will opens for us the way to create new variants by providing a name and also will hook the gateway supergraph schema updates on deploy - this is definitely a must-try option instead of if/else config in the apollo server config code, with the scary "pollInterval" config :)

@abernix
Copy link
Member

abernix commented Aug 20, 2021

I'm not against the other proposed options here but I like the way that create and delete compliment each other and are clear in their naming.

They are small and intuitive building blocks!

@EverlastingBugstopper
Copy link
Contributor

I think the best way forward here is going to be to create the graph create and graph delete commands first. Then when it comes time to address #698 and we introduce supergraph publish, we can re-use the logic and semantics we use for graph create and graph delete to ensure a consistent experience.

@abernix
Copy link
Member

abernix commented Aug 24, 2021

@EverlastingBugstopper I support that. In the future step, I suspect we may want to have first-class support in some of the APIs for doing this more atomically within a single larger command (if we don't have that today?), but I still believe that comes as a subsequent step.

@prasek Are you okay with this as an initial stone-paving step?

@EverlastingBugstopper
Copy link
Contributor

EverlastingBugstopper commented Aug 26, 2021

I think there are maybe a few issues we'll need to address as part of this approach:

  1. rover graph publish this-graph-exists@this-variant-does-not-yet-exist will create a new variant
  2. rover subgraph publish this-graph-exists@this-variant-does-not-yet-exist will create a new variant only if the --convert flag is passed (see Unable to create a new variant of a federated graph with rover subgraph publish #661)
  3. There currently isn't a way in Apollo's API to create a new "empty" variant - as in one without a schema at all (we could possibly paper over this by creating new variants that are duplicates of current but that may lead to some unexpected behavior)

I think after some thought and investigation that what I've proposed here will still be the cleanest method moving forward, but we'll need to:

  1. Provide a way to create an empty variant and figure out what happens when empty variants are fetched
  2. Start erroring on publish invocations for variants that do not yet exist and advertise this breaking change well, possibly by starting w/a stderr warning, and errors in future versions

@EverlastingBugstopper
Copy link
Contributor

Hey @jsegaran or @zionts - does the approach I have outlined in no. 3 in that last comment make any sense? Are there any pitfalls I should be aware of if we start out a new variant based on current?

@ndintenfass
Copy link
Contributor

Are there any pitfalls I should be aware of if we start out a new variant based on current?

The history of a variant created this way may be kinda janky. If current were federated would that also mean we'd copy it's subgraphs? Or would you manually grab the api schema and publish just that (which could end up being a strange state)? Would you also copy the query URL associated with current?

@EverlastingBugstopper
Copy link
Contributor

The history of a variant created this way may be kinda janky

yeah this was something I had considered but thought it might be fine if Apollo's current was associated w/git's main branch, but that's a pretty loaded assumption.

If current were federated would that also mean we'd copy it's subgraphs? Or would you manually grab the api schema and publish just that (which could end up being a strange state)?

This is also somewhat strange, especially since graphs themselves can contain multiple federated variants and multiple unfederated variants. Ideally you'd be able to graph new and then either subgraph publish or graph publish to "decide" if that variant is federated or not.

Would you also copy the query URL associated with current?

I'd say no to this one, likely should start out null and then have the URL setting happen within publish.


These concerns do seem to highlight some unfortunate realities. I really would like to provide some way for users to create/delete graph variants, but it just seems so coupled to the Studio front end it makes it quite difficult to reason about from a CLI.

Would love to hear if anybody has any workarounds/a way we can provide this functionality today as there is a very real need for this!

Unfortunately I think that my initial judgment here is that the investigative work we've done here can give some context towards the design of the new graph registry API, but that we should maybe hold off on this until these concerns can be addressed properly.

@prasek
Copy link

prasek commented Sep 14, 2021

rover supergraph publish my-graph@ci --config supergraph.yaml --ensureNewGraphRef

@AndreyIlyunin: Yep. The single command is the most attractive format for working with variants from the CI.
Not sure about the configs with the list, as it looks just like the same replacement for the serviceList, but if this will opens for us the way to create new variants by providing a name and also will hook the gateway supergraph schema updates on deploy - this is definitely a must-try option instead of if/else config in the apollo server config code, with the scary "pollInterval" config :)

@EverlastingBugstopper: I think the best way forward here is going to be to create the graph create and graph delete commands first. Then when it comes time to address #698 and we introduce supergraph publish, we can re-use the logic and semantics we use for graph create and graph delete to ensure a consistent experience.

@abernix: @prasek Are you okay with this as an initial stone-paving step?

It depends on how long it will take for graph create/delete commands to be available -- if it turns out to be a blocking issue for a while, and we could otherwise implement rover supergraph publish my-graph@ci --config supergraph.yaml --ensureNewGraphRef to solve the problem right now with an end-state command syntax, and then I'd be in favor building a great dev experience in rover more quickly using the available Studio APIs vs. getting blocked on an ideal future state.

/cc @ndintenfass @jstjoe @zionts @jsegaran

@abernix
Copy link
Member

abernix commented Sep 15, 2021

@prasek: Reading the above, I'm understanding that @EverlastingBugstopper already followed up on the approach you quoted, noting its faults: create on its own is problematic (empty graph content) and supergraph publish needs API changes (to support the creation).

I believe at this point we're at the point of needing API changes to proceed. I don't think those API changes for supergraph publish are necessarily difficult or complex. It's possible we should work with @caydie-tran on this.

@EverlastingBugstopper
Copy link
Contributor

After thinking about this a bit more, I have a new proposal that should be a bit less work for us and doesn't require Gravity to make any changes.

  1. Don't require people to pass --convert when running subgraph publish on a graph whose variant does not exist
  2. When subgraph publish or graph publish is run on a variant that does not exist, print a message to stderr informing the user that a new graph variant is being created
  3. Create a new graph delete command that prompts the user (or takes a --force flag) that will delete a single variant of a graph.

This should unblock folks for now and make it a bit easier to work w/the registry in CI pipelines that need to create and destroy per-branch variants.

In the future, should we want to separate publish and create semantics, we can still move forward with that by disallowing creation on publish invocations, likely by phasing it out over time. Start with a warning that this functionality will be removed and after maybe a month we'd hard error.

@EverlastingBugstopper EverlastingBugstopper removed the triage issues and PRs that need to be triaged label Oct 19, 2021
@EverlastingBugstopper EverlastingBugstopper added this to the 0.4.0 milestone Oct 19, 2021
This was referenced Nov 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 🎉 new commands, flags, functionality, and improved error messages
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants