Closed joel-burton closed 1 year ago
reading through this proposal i noticed that you have a proposal on the commands which is great but i also think we're missing a section on which parts of the codebase/files/sections of code you're looking to add more surface area to. have you looked into how the API interacts with Rover?
@joel-burton thanks for putting this together!
This all makes sense to me and fits in with the existing rover design, and I'm happy to pair with anyone taking on implementation here.
Taking a step back - I'd love if you could take a minute to look over my proposal here around creating a .apollo
configuration directory and maybe we can brainstorm how something like this could fit in with that design long term. As time goes on, there becomes more opportunity for the exact state drift you mention here and I think it would be great to provide a terraform-y paradigm with rover and GraphOS/the platform API and give a place for these kinds of configuration profiles to live. We've had requests for managing operation checks from Rover and also managing cloud router configuration. It would be great to take some time to think holistically on a design that can suit all of these needs without shifting the variant management to customer's CI builds. One rover apply
or something like that feels much stronger than stringing together individual fetch/publish commands like the existing commands require.
@caydie-tran re: that last q:
contract
directory in ./crates/rover-client/operations
- typically each command has its own directory.contract
command directory in ./src/command
FYI @zionts opened an issue about this a while back.
Hi yall! Gonna take over the first part of this proposal, namely:
After reading through the specifics outlined here, I'm mostly fine with it. There's a few things though I'd like to change.
rover contract publish
For rover contract publish
, we need a new optional boolean argument --hide-unreachable-types
(for a new configuration option we introduced for contracts).
For rover contract publish
, it would also be nice for there to be a new optional argument --config
with the following properties:
rover contract fetch
.--source-variant
, --include
, --exclude
, and --hide-unreachable-types
) are disallowed. That is, these sets of arguments are mutually exclusive.-
, it accepts input from stdin
, similar to the --schema
argument in Rover.The main worry here is that if we don't introduce --config
, users will write their own scripts to parse the format and pass it to Rover. And if that happens, we'll have to worry about breaking user scripts when we make changes to the format.
E.g., if we just added a new field to the document for specifying some new configuration option, and if scripts didn't see this and pass it to Rover, then the scripts would be silently dropping the new option which may result in bugs for users.
rover contract fetch
For rover contract fetch
, there's a few changes that would be good here:
rover contract fetch graph@old-contract | rover contract publish graph@new-contract --config -
; they'd need a yq
in the middle to patch the document.
filterConfig
, it would be cleaner to flatten it out for simplicity, so the format would look like e.g.:
source_variant: graph@base-variant
include:
- public
exclude:
- private
hide_unreachable_types: true
A few other things worth noting:
source_variant
accepts a graph ref, although the graph ID must always match the base variant's, so this is redundant. We could use
source_variant_name: base-variant
include:
- public
exclude:
- private
hide_unreachable_types: true
instead, but not sure how folks feel about it. (If we use source_variant
instead of source_variant_name
, it would mean a bit more yq
patching for users in some cases.)
config_version: contracts/v1
. I've heard though we should add that in after the first breaking change, and not now, so I think it's scoped out.This all sounds pretty reasonable to me! Few notes:
When specified, all other config arguments (namely --source-variant, --include, --exclude, and --hide-unreachable-types) are disallowed. That is, these sets of arguments are mutually exclusive.
I don't think we should make these mutually exclusive. Typically in CLIs, you'll see precedence rules like argument > environment variable > configuration file
. This is a pattern we are using for rover dev
's router configuration file and we should do the same here.
Agree on YAML.
Re: variant config. Can you have multiple variants defined in a single config file? Should we be able to? What does the workflow look like for handling multiple contracts across multiple variants? Should they be in separate files and tracked differently in version control?
I think if I were to take an opinionated stance here, I would say that we should allow multiple variants in one contracts.yaml
that all get fetched and published at the same time so you don't have to keep track of all the different combinations in hundreds of different files, but I'm willing to discuss this further.
Part of me really thinks we should figure out "terraform-y" style projects before or as a part of this work, since we are really starting to put some config burden on our users. We have supergraph config, subgraph config, router config, contract config, we want to do things like introduce more lint features and other check types, and even operation collection management. All of this can be really overwhelming and I'm worried that if we continue to just sorta cobble stuff together one by one that we'll leave developers frustrated and that they have to spend time writing their own tooling to integrate with us.
I don't think we should make these mutually exclusive. Typically in CLIs, you'll see precedence rules like
argument > environment variable > configuration file
. This is a pattern we are using forrover dev
's router configuration file and we should do the same here.
That sounds fine to me, as long as there's precedent implemented elsewhere I can look at that code and copy whatever pattern they're using. (I imagine it's something like YAML parse, patch, and build, and then submitting that YAML to Studio backend.) If it takes a lot of work though/is non-trivial, it may get scoped for later/follow-up work.
Re: variant config. Can you have multiple variants defined in a single config file? Should we be able to?
Long-term I think we want to allow this somehow (similar to Kubernetes resource manifests), although I don't think it's in scope to support that for this initiative.
What does the workflow look like for handling multiple contracts across multiple variants? Should they be in separate files and tracked differently in version control?
Indeed, if they tried to store configuration documents in version control, they would need to be in separate files.
I think if I were to take an opinionated stance here, I would say that we should allow multiple variants in one
contracts.yaml
that all get fetched and published at the same time so you don't have to keep track of all the different combinations in hundreds of different files, but I'm willing to discuss this further.
I think long-term we want folks to be able to store variant configurations in version control to better support config-as-code, and in such a format that they could store multiple YAML configuration documents in one file. I don't think it's in scope for this particular project though.
My guess is that when we do introduce such a format, there'll be some YAML key/value pair we can look up to tell that it's using such a format (akin to apiVersion
in Kubernetes resource manifests, or maybe magic bytes/numbers in file formats).
Part of me really thinks we should figure out "terraform-y" style projects before or as a part of this work, since we are really starting to put some config burden on our users. We have supergraph config, subgraph config, router config, contract config, we want to do things like introduce more lint features and other check types, and even operation collection management. All of this can be really overwhelming and I'm worried that if we continue to just sorta cobble stuff together one by one that we'll leave developers frustrated and that they have to spend time writing their own tooling to integrate with us.
I do know long-term there's a lot of interest in supporting "terraform-y" configuration/config-as-code, but I'm not sure when we're planning to take that on in our roadmap/what the product prioritization is. @papollosc might be able to give more insight here.
Part of me really thinks we should figure out "terraform-y" style projects before or as a part of this work, since we are really starting to put some config burden on our users.
Totally understand the sentiment here and we have been calling this config-as-code; however, it would have to make its way to being a "big rock" initiative in order for us to treat it as such. The original goal of this project, configuring contracts via rover, was to solve specific use cases that customers were asking for. I think until config-as-code becomes a big rock which I have no doubt in my mind that it will be but not for H1, we should just build out the original proposal.
Agree on all fronts here. Avery and I just talked and we came to good interim solution until we can take on config-as-code as a big rock.
For the publish
let's keep it to --exclude
and --include
arguments.
And instead of fetch
, let's do something like a describe
, the output of which can be how the contract variant has been configured - source variant, include and exclude tags etc. in a more text format than a config format to avoid any confusion and distress future work.
That works for me. If we do rover contract describe
instead of fetch
, that'll discourage folks from trying to parse the text output in scripts (and at the very least, it'll make it clear that script failures on text changes are on scriptwriters to resolve).
So then to summarize, the amendments to the proposal are:
rover contract publish
will have a new argument named --hide-unreachable-types
(but will NOT have a --config
argument).rover contract fetch
will be changed to rover contract describe
, and will print a human-readable description of the contract's configuration.Makes sense to me! Folks will still be able to script the output of rover contract describe
if they pass --output json
(you'll have to make a RoverOutput
variant for contract stuff that can serialize to JSON and a version for the formatted text) and this should be easily integrated into existing pipelines that wrap these JSON structs. 👍🏼
Rover x Contracts
Old design brief, from contract ideation
Note
What problem are we solving?
This addition to contracts primarily aims to solve two problems:
Initial proposal is around establishing a new namespace for working with contract variants, starting with commands for creating, updating and reading contract configuration, followed by a second proposal focused on running contract checks via Rover.
For this first proposal we won't be adding the ability to manage contracts via configuration files. There's a couple reasons for this:
There's the definite possibility of adding these in the future, when we have a better understanding of they would be used and managed, but for the initial release it's out of scope.
Contract Configuration
Contract configuration has 3 elements:
@inaccessible
in the supergraph schema)Proposed Commands
contract publish
Analogous to
subgraph publish
, meant to create and update contract variants, and by default launch & publish. However,subgraph publish
pushes out schema updates, where this new command would be pushing out new contract configurations to update contract schemas.Under the hood this command will simply be passing the argument to and calling the
upsertContractVariant()
mutation. This will also require adding this mutation to the platform-apiInput
--source-variant
(optional)upsertContractVariant()
mutation. I think the easiest course of action would be to make that field optional on the mutation as well, instead of adding extra logic to try and populate the value when it's absent.--include
&--exclude
(optional)clap
--no-launch
Output
contract fetch
Similar to other fetch commands,
rover contract fetch my-graph@public-contract
would return the contract configuration to thestdout
output.The main use case here is to be able to easily read & review a given contract's current configuration. This could also be used in a script with
contract publish
to clone a variant.Input
rover contract fetch my-graph@public-contract
Output
Out the the json object returned by the API. Formatted as inputs to the
upsertContractVariant()
mutation.Other Functionality & Surface Area to Consider
contract check
A command to be added in a follow up to allow running checks on a specific contract. Details will be added in a dedicated proposal for this command.
Deleting a Contract
You can delete a contract variant using
rover graph delete
.Fetching Contract Schemas
graph fetch
andsupergraph fetch
should be used for fetching API and Supergraph schemas, respectively.contract list
Should we consider adding a command to list all contract variants based on a given source variant? Not super important for this proposal, but something to consider as we're pushing for feature parity with contract variants.
contract filter
In the future will we need a command analogous to
supergraph compose
for contracts? What workflows would this support? Out of scope for this proposal, but want to mention it as a consideration.