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

feat: Add rover contract describe and rover contract publish #1475

Merged
merged 21 commits into from
Jan 20, 2023

Conversation

sachindshinde
Copy link
Contributor

@sachindshinde sachindshinde commented Jan 18, 2023

This PR implements the first phase of #1421

That is, this PR:

  • Implements rover contract describe, which prints out a human-readable description of the configuration of a contract variant.
Describe the configuration of a contract variant from the Apollo graph registry

Usage: rover contract describe [OPTIONS] <GRAPH_REF>

Arguments:
  <GRAPH_REF>
          <NAME>@<VARIANT> of graph in Apollo Studio. @<VARIANT> may be left off, defaulting to @current
  • Implements rover contract publish, which allows creating a contract variant (or updating its configuration) and optionally triggering a launch.
Publish an updated contract configuration to the Apollo graph registry and trigger launch in the graph router

Usage: rover contract publish [OPTIONS] <--include-tag <INCLUDE_TAG>|--no-include-tags> <--exclude-tag <EXCLUDE_TAG>|--no-exclude-tags> <--hide-unreachable-types|--no-hide-unreachable-types> <GRAPH_REF>

Arguments:
  <GRAPH_REF>
          <NAME>@<VARIANT> of graph in Apollo Studio. @<VARIANT> may be left off, defaulting to @current

Options:
      --source-variant <SOURCE_VARIANT>
          The source variant name for this contract variant. Once set, this cannot be changed

      --include-tag <INCLUDE_TAG>
          List of tag names to include in the contract schema (e.g. '--include-tag foo --include-tag bar'). To specify an empty list, use --no-include-tags instead

      --no-include-tags
          Use an empty include list of tag names for the contract schema. To specify a non-empty list, use --include-tag instead

      --exclude-tag <EXCLUDE_TAG>
          List of tag names to exclude from the contract schema (e.g. '--exclude-tag foo --exclude-tag bar'). To specify an empty list, use --no-exclude-tags instead

      --no-exclude-tags
          Use an empty exclude list of tag names for the contract schema. To specify a non-empty list, use --exclude-tag instead

      --hide-unreachable-types
          Automatically hide types that can never be reached in operations on the contract schema

      --no-hide-unreachable-types
          Do not automatically hide types that can never be reached in operations on the contract schema

      --no-launch
          Do not trigger a launch in Studio after updating the contract configuration

Some notes:

  • The API for upserting a contract's configuration does not allow specifying a particular key as null to indicate "don't change this key's value". This means that most configuration is required to be specified each time the command is run.
    • The exception is --source-variant, which the API allows to be optional.
    • Follow-up work will take on making these configuration keys nullable in the API. I've set up the Rover code such that making the switch shouldn't be too much work.
      • Should mainly be changing the ArgGroups to use required(false), updating the runner.rs/types.rs, and removing the None check in run().
      • Note that instead of saying "not specifying --include or --no-include means use the pre-existing value", we could also introduce a new argument e.g. --keep-include that explicitly keeps the pre-existing value, and add --keep-include to the ArgGroup instead of making the ArgGroup not required. (Similar situation for the other configuration options.) This is something to consider in the follow-up work though/doesn't need to be considered here.

@sachindshinde sachindshinde force-pushed the sachin/new-contracts-commands branch 2 times, most recently from e4c7393 to d67ead8 Compare January 18, 2023 18:20
@sachindshinde sachindshinde changed the title Sachin/new contracts commands feat: Add rover contract describe and rover contract publish Jan 18, 2023
@EverlastingBugstopper
Copy link
Contributor

After reading the docs PR (and re-learning about contracts) it looks like these includes/excludes are only related to tags, making me think we should rename --include and --no-include to --include-tags and --no-include-tags if we think we might have different filter kinds down the line.

Alllsssoo, I think it would be super helpful if rover contract describe spit out a suggestion for how to create that same contract (and let's tap @StephenBarlow on the shoulder for some copy help).

$ rover contract describe my-graph@my-contract-variant
Fetching description for configuration of my-graph@my-contract-variant using credentials from the default profile.

Configuration Description:

"my-graph@my-contract-variant" is derived from "my-graph@my-source-variant".

Included tags:

- "foo"
- "bar"

Excluded tags:

- "baz"

All unreachable types are hidden.

View the variant's full configuration at https://studio.apollographql.com/graph/my-graph/settings/variant?variant=my-contract-variant

💡 You can re-create this contract by running `rover contract publish my-graph@my-contract-variant --source-variant my-source-variant --include foo --include bar --exclude baz --hide-unreachable-types`

I think that callout at the end would be super helpful, especially for folks with existing contracts who want to port their contracts to CI.

@sachindshinde
Copy link
Contributor Author

sachindshinde commented Jan 18, 2023

@EverlastingBugstopper

After reading the docs PR (and re-learning about contracts) it looks like these includes/excludes are only related to tags, making me think we should rename --include and --no-include to --include-tags and --no-include-tags if we think we might have different filter kinds down the line.

I'm down to rename to --include-tag and --no-include-tags. I'm not aware of any other kinds of including/excluding we'll be doing, but it definitely clarifies (it's not that much extra to type either for users).

Alllsssoo, I think it would be super helpful if rover contract describe spit out a suggestion for how to create that same contract (and let's tap @StephenBarlow on the shoulder for some copy help).

I think changing the description to be formatted like

"my-graph@my-contract-variant" is derived from "my-graph@my-source-variant".

Included tags:

- "foo"
- "bar"

Excluded tags:

- "baz"

All unreachable types are hidden.

is fine, I can file a quick backend PR for that.

Agreed that giving folks a command to recreate the contract would be nice, although it'll take adding another field to the platform API. Would you be down if we shift that to follow-up work?

@sachindshinde sachindshinde force-pushed the sachin/new-contracts-commands branch from a952dc3 to 3a354ac Compare January 19, 2023 04:06
@sachindshinde
Copy link
Contributor Author

@EverlastingBugstopper
PR should be ready for review again. Some notes:

  • As per @caydie-tran 's request, I've taken the changes she and I pushed up in Introduce contracts commands to Rover docs #1473 and cherry-picked them onto this PR.
  • I've updated the arguments to be --include-tag/--no-include-tags and --exclude-tag/--no-exclude-tags (using singular for the non-zero case, since users specify one tag name per invocation), and updated docs accordingly.
  • I've updated the configuration description output to be formatted in the styling you've suggested above (with some tweaks to wording). I've updated the example in the docs as well.

I'll file tickets for the remaining follow-up work (adding keep-old-value capabilities and showing users a command to recreate a variant) sometime tomorrow.

@EverlastingBugstopper
Copy link
Contributor

Sounds good to me on getting the command construction in a follow-up. That being said, I think it might be easier to just include that code directly in Rover itself. If we keep backwards compatibility, it should be fine to just construct that on Rover's side so we can update usage down the line but old versions will continue to output the correct command suggestion.

@EverlastingBugstopper
Copy link
Contributor

Two API errors I ran into during QA that were not the nicest:

$ rover contract publish averys-supercloud@contract --source-variant averys-supercloud@current --hide-unreachable-types --no-include-tags --no-exclude-tag
s
Publishing configuration to averys-supercloud@contract using credentials from the default profile.

error: Exception while fetching data (/graph/upsertContractVariant) : NOT_ALLOWED_BY_USER_ROLE
$ rover contract publish averys-supercloud@contract --source-variant averys-supercloud@current --hide-unreachable-types --no-include-tags --no-exclude-tags --profile github
Publishing configuration to averys-supercloud@contract using credentials from the github profile.

error: Exception while fetching data (/graph/upsertContractVariant) : Your accounts does not include access to contracts

@sachindshinde
Copy link
Contributor Author

@EverlastingBugstopper

Sounds good to me on getting the command construction in a follow-up. That being said, I think it might be easier to just include that code directly in Rover itself. If we keep backwards compatibility, it should be fine to just construct that on Rover's side so we can update usage down the line but old versions will continue to output the correct command suggestion.

Makes sense. When we get around to YAML configs, I imagine there will be a way to fetch a config file from the backend, and if Rover sees it can't parse it fully (e.g. sees new options/a version it doesn't know about), it could notify the user to upgrade to latest.

Two API errors I ran into during QA that were not the nicest:

Yeah, we don't have a nice system/pattern for permission/capability errors unfortunately 😢

#[clap(flatten)]
profile: ProfileOpt,

/// The source variant name for this contract variant. Once set, this cannot be changed.
Copy link
Contributor

Choose a reason for hiding this comment

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

i tried specifying --source-variant as averys-supercloud@current instead of just current and got an expected but unfortunately confusing error message as a result:

$ APOLLO_REGISTRY_URL="https://api-staging.apollographql.com/graphql" rover contract publish averys-supercloud-staging@contract --source-variant averys-su
percloud-staging@current --hide-unreachable-types --no-include-tags --no-exclude-tags
Publishing configuration to averys-supercloud-staging@contract using credentials from the staging profile.

error[E040]: While publishing the contract configuration and triggering launch, the following error occurred:
The source variant "averys-supercloud-staging@current" does not exist.

we could add in a check if there is an @ included, and if there is, run GraphRef::parse. then we just use the extracted variant name if it parses correctly and the graph id matches the contract graph id you're trying to create

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, we have some existing legacy variants in production that use the @ symbol in their variant name, so we can't unambiguously tell whether a string that represents a variant name or a graph ref is one or the other 😢 .

We could have two arguments: one named --source-variant and one named --source-variant-name (the first taking a graph ref, and the second taking a variant name), and put them in an optional ArgGroup. Would that be fine here?

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm - could we just call GraphRef::parse() on it directly and see if it spits out a variant? there's a regex in there that could help. I think the existing --source-variant argument is perfectly fine and we shouldn't add in --source-variant-name in addition, I'm just anticipating that this could be a common mistake (typing the full graph ref instead of just the variant) that we could handle a bit better. Specifically, seeing the text The source variant "averys-supercloud-staging@current" does not exist. for a variant that does exist is really confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Giving an update here -- we're going to change the backend API to accept both graph refs and variant names for sourceVariant.

@EverlastingBugstopper EverlastingBugstopper merged commit 7e0da30 into main Jan 20, 2023
@EverlastingBugstopper EverlastingBugstopper deleted the sachin/new-contracts-commands branch January 20, 2023 17:08
EverlastingBugstopper added a commit that referenced this pull request Jan 24, 2023
# [0.11.0] - 2023-01-24

## 🚀 Features

- **Manage contract configuration - @sachindshinde, #1475 fixes #1421**
 
Rover now includes two commands for creating, modifying, and reading
contracts: `rover contract publish` and `rover contract describe`.
Further documentation can be found
[here](https://www.apollographql.com/docs/rover/commands/contracts).

- **Easier file output with new `--output` argument - @gocamille, #1413
fixes #1212**

The `--output` argument has long been used to configure the format of
Rover's output. i.e. `--output json` configures Rover to print its
output as a JSON object. This argument has been _renamed_ to `--format`
while maintaining backwards compatibility. `--format json` should be
used to configure the format of Rover's output, and the `--output`
argument allows you to specify a file to print the output to. Adding
`--output schema.graphql` to a `rover subgraph fetch` command will
output your schema to a file. `--output data.json --format json` will
output the command data to `data.json`. `--output json` will still work
by itself but will now print a warning, and `--output [json|plain]` does
not work with `--format [json|plain]`. Further documentation can be
found
[here](https://www.apollographql.com/docs/rover/conventions#output-to-a-file).

- **Adds `--router-config` to `rover dev` - @EverlastingBugstopper,
#1446 fixes #1373, #1345, and #1468**

The new `--router-config` argument for `rover dev` allows you to pass a
[router configuration file]() on startup that can configure settings
such as header propagaion and CORS policies. Further documentation can
be found
[here](https://www.apollographql.com/docs/rover/commands/dev/#advanced-configuration).

- **Auto-update router versions in `rover dev` - @EverlastingBugstopper,
#1432**

`rover dev` will automatically use the version of the router specified
in [this plugin
file](https://github.com/apollographql/rover/blob/main/latest_plugin_versions.json)
instead of a hard coded version.

## 🛠 Maintenance

- **Better error and help text for ELv2 license argument - @DoumanAsh,
#1456 fixes #1455**

The help text for the `--elv2-license` argument now includes the
expected value for the argument, in addition to the error message
returned when the argument is needed but not passed.

- **Updates the Ariadne template URL - @patrick91, #1439**

- **Updates `./examples/supergraph-demo` to `@apollo/server` v4, and
removes `./examples/dev` - @EverlastingBugsttopper, #1442 fixes #1440
and #1441**

- **Updates dependencies - @EverlastingBugstopper, #1481, #1450**

  `apollo-parser` 0.3 -> 0.4
  `base64` 0.13 -> 0.21
  `git2` 0.15 -> 0.16
  `graphql_client` 0.11.0 -> 0.12
  `serial_test` 0.9 -> 1.0
  `os_info` 3.4 -> 3.5
  `os_type` 2.4 -> 2.6
  `termcolor` 1.1 -> 1.2
  `tokio` 1.21 -> 1.24

## 📚 Documentation

- **Fixes a link to schema check example - @MayCXC, #1431**
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.

3 participants