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.

Sign up for GitHub

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

Jump to bottom

RFC: simplify subgraph getting started and federation #10303

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

RFC: simplify subgraph getting started and federation #10303

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7ab3fa5
20240620-simplify-subgraph-getting-started-and-federation
nullxone Jun 20, 2024
89495d5
Clarify details on env overrides and target flags
nullxone Jun 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions v3/rfcs/dx/20240620-simplify-subgraph-getting-started-and-federation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
# DX: simplify subgraphs for getting started and federation workflows

Status: Review required

## subgraph init command
What this is: as a part of making it easier for people to work through the getting started guide, or to scaffold project files, allow the user to go from scratch to an API with only CLI commands.

init-ing a supergraph or connector will create files for both local and cloud environments. This is about what happens for the subgraphs.

```bash
# creates a subgraph folder and subgraph.yaml file
ddn subgraph init app
Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming this command is run in a DDN project directory (where we can locate hasura.yaml)? Then we create the app directory at the root of the project? It would not make a lot of sense if the directory in created in the CWD. We should also think about the variant for federation use case, where we init a standalone subgraph (thus creating hasura.yaml, supergraph.local.yaml, the globals directory, engine, docker-compose files etc)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, let's keep the existing behaviour of putting next to hasura.yaml.

Let's also introduce a --dir flag to explicitly give the directory like we do with connectors.


# add subgraph.yaml to the supergraph.yaml file and create an env file for the env
ddn supergraph add subgraph --supergraph supergraph.local.yaml --subgraph app/subgraph.yaml
touch app/.env.app.local
Copy link
Contributor

Choose a reason for hiding this comment

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

From a primitives PoV, touch to create a .env file makes sense. But this is gonna be different for UNIX and powershell. Do during subgraph init, can we by default initialize a .env.app.local file (and add it to the subgraph.yaml as well)? The rationale is we init a subgrph, do local dev and then go to cloud, so a local .env file is absolutely needed here, so why not create it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ddn connector-link add takes a --configure-host flag. Add a --env-file flag here as well. Creates the env file if not present.

Now we can remove the touch command.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The command should be ddn subgraph add

Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we initialize a subgraph.cloud.yaml and subgraph.local.yaml` when we initialize a subgraph along with the corresponding .env files? The files will be the same except for the .env file.

The advantages of doing so are:

  1. We generate local and cloud files for supergraph and connector. We also have to generate local and cloud file for the globals subgraph.yaml. Doing the same when initializing a subgraph would keep the behaviour consistent with everything else we are doing.
  2. It would make the commands much more simpler as users don't need to pass a --env-file flag. Advanced users can still do it if they want but for simple use cases and getting started, not having to pass an additional flag would keep things simpler and easier to grasp. The fact that it is more consistent as mentioned in (1) would also make it more easier to grasp.

The only drawback is that we'll have 2 files that are the same except for the .env file. However since this is how it's done in all other cases and since this will eliminate the need to add an extra flag in all the commands, this seems like the more simpler approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

The command should be ddn subgraph add

IMO, this sounds like we are adding something to the subgraph. I feel like ddn supergraph add subgraph as in the original makes more sense although it's more verbose. Or were you talking about something else?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let's go with

  • single subgraph.yaml
  • scaffold both .env.app.local and .env.app.cloud

This does not introduce additional subgraph.yaml files with no differences, but also makes the model clear to the user.

Copy link
Collaborator Author

@nullxone nullxone Jun 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

ddn subgraph add --supergraph
is along the lines of
ddn model add --subgraph
and so is consistent

The alternative here could be
ddn supergraph subgraph add
Which is more verbose and doesn't really add much value. We've essentially raised subgraph to a top level subcommand exactly like models.


# create the supergraph build
ddn supergraph build --supergraph supergraph.yaml --env-file app/.env.app.local
Copy link
Contributor

Choose a reason for hiding this comment

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

In supergraph build, there could be multiple subgraphs and each can have its own env file. With this command, we cannot determine, which env file belongs to which subgraph.yaml. We might need to do something like the prefixing we did for the -e flag like --env-file <subgraph-Name>=<path-to-env-file>. Looks ugly though

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right. Can we do --env-file <subgraph>:<env file>.

Can do --env <subgraph>:<key>=<value> for consistency.


# deploy to cloud
touch app/.env.app.cloud
ddn connector build create --connector connector.yaml --target-env-file app/.env.app.cloud
Copy link
Contributor

Choose a reason for hiding this comment

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

We might not need a touch here. If the command is like ddn connector build create --connector mydb/connector.yaml --target-subgraph app/subgraph.yaml --target-connector-link mypg --target-env-file app/.env.app.cloud . If the --target-env-file is not provided, we use the envFile specified in subgraph.yaml. If the flag is provided, we create the file if it does not exist and write it there

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree, can do without touch. Makes for better cross platform docs as well.

```

Order of env
- specify either as --env flags or --env-file flags, which can be specified multiple times
- all flags override the env specified in the subgraph.yaml file
- later flags override earlier flags

## --subgraph flag should take the file name

What this is about: on introducing federation, we realize that the primitive is the subgraph not the supergraph. The following commands are updated to consider the subgraph.yaml file instead of specifying a supergraph.yaml file and the subgraph by name.

```bash
ddn connector init mydb --subgraph app/subgraph.yaml --hub-connector hasura/postgres
Copy link
Contributor

Choose a reason for hiding this comment

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

These commands make sense. This would mean that --supergraph flag goes away in these commands. This is a breaking change. I guess we should be ok with that. Just calling it out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this is breaking. Confirmed.

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 want to/ would it be possible to make it backwards compatible by allowing --subgraph to accept either name or filepath. That way existing users can continue to use the command and new users will be able to use the new approach. We can even show a deprecation notice if someone passes subgraph name in flag and maybe even remove the behaviour a few versions down the line.

Not sure if any of this is worth it but just putting it out here for thought.


ddn connector build create --connector mydb/connector.yaml --target-subgraph app/subgraph.yaml --target-connector-link mypg # use env file from subgraph.yaml

ddn connector build create --connector mydb/connector.yaml --target-subgraph app/subgraph.yaml --target-connector-link mypg --target-env-file app/.env.app.cloud

ddn model add --subgraph app/subgraph.yaml --connector-link mydb --name Album

ddn connector build get --connector-name mypg --subgraph-name app --project proj-1234
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we use --subgraph here also and we could get the subgraph name from the definition?

I get that the subgraph mentioned here is the subgraph in the cloud project. If so, then should the flag be --project-subgraph so that it will be similar to the command related to the same concept ddn project subgraph.


ddn connector build get --connector mypg/connector.yaml
```