Create --variant flag, deprecate --tag flag #1849

zionts · 2020-03-25T04:46:00Z

This PR changes all commands that previously used a --tag flag to additionally include a --variant (alias -v) flag that can be used instead. These two different flags are exclusive, so no command can pass both a --tag and a --variant. The PR may look daunting at a glance, but please go commit-by-commit to see the progression, and I hope that will make it really manageable! These changes were QA'ed manually, and all tests were updated and inspected to ensure that they continued to work as expected. Some other small changes were added, which are noted in commit messages.

This is rebased off of #1847 , so check that one out first :)
See apollographql/apollo#819 for the corresponding documentation update 🎉

Notably, they include:

Changing messages in service:check to include @variant where appropriate
Changing (buggy) logic in several places that were using flags.tag directly rather than config.tag, in GraphQL commands. This could have caused issues for people that were not using a --tag flag but instead were using service { name: 'blah@foo' }, in that their command would fail unexpectedly.

Error message when both --tag and --variant are set:

client:push with --tag (note deprecation warning):

client:push with --variant:

client:check with --tag:

client:check with --variant:

client:codegen with --tag:

client:codegen with --variant:

service:push (federated) with --tag:

service:push (un-federated) with --variant:

service:check (un-federated) with --tag:

service:check (federated) with --variant:

service:list with --tag:

service:list with --variant:

service:list with neither --tag nor --variant, just to show that defaults work :)

Also throwback to #1398 🎉 Credit to former me & @jgzuke for inspiration, along with whomever made the config magic (which I think may have been @trevor-scheer?), which I relied on heavily here :)

TODO:

Update CHANGELOG.md* with your change (include reference to issue & this PR)
Make sure all of the significant new logic is covered by tests
Rebase your changes on master so that they can be merged easily
Make sure all tests and linter rules pass

*Make sure changelog entries note which project(s) has been affected. See older entries for examples on what this looks like.

JakeDawkins · 2020-03-25T18:07:15Z

packages/apollo-language-server/src/config/config.ts

@@ -162,12 +162,12 @@ export class ApolloConfig {
    return configs;
  }

-  set tag(tag: string) {
-    this._tag = tag;
+  set variant(tag: string) {


My only concern is if this breaks anything in vs code. But I don’t think it should

I admit I didn't test with VS code, but mainly because I don't know how to test a local build! Is there a QA practice we go through before release?

Not in a consistent way! I’ll run through all this before releasing, but I don’t expect any issue here since it only interfaces with the language server anyway :)

JakeDawkins · 2020-03-25T18:12:07Z

CHANGELOG.md

@@ -3,7 +3,14 @@
 ## Upcoming

 - `apollo`
-  - <First `apollo` related entry goes here>
+  - https://github.com/apollographql/apollo-tooling/pull/1849


Formatting of this entry isn’t consistent with what we have but I’m not concerned too much there, and that’s non blocking. I can amend before releasing since I’ll have to do that anyway

These default flags were still referencing Apollo Engine instead of Graph Manager (even though some were hidden). Additionally, this updates the docs to be more accurate. Happy to take this commit into another PR if you'd like!

This change supports the variant flag (with character -v) in addition to the "tag" flag, which allows us to start using the new terminology. The '--tag' flag is still supported, and there is verification that the two sibling flags cannot be used together.

Also remove an unused variable in service:list

Also update description of the flags

Also simplify an import in apollo-language-server Also add a XXX comment to service:download to illustrate a problem

Additionally, this updates the text output in the service:check command to consistently use graph@variant everywhere

This is a find and replace all, and I verified every command still works after this (both with --tag and --variant). Will post screenshots of how everything looks in the PR :)

A previous commit added `@variant` everywhere, and these snapshot updates should be ONLY that.

All of the client commands share common flags, so no further changes are required. Also, all of them use config.variant, so yay! :)

Remove the deprecation warning from specific callsites and rely on the deprecation warning appearing when the config.variant value is set

Also, this changes the default value of the flag, but it does NOT change the behaviour, since `config.variant` always defaults to "current" if flags and configuration are not set. The reason this was done is because flags.tag will be non-null if there is a default, which causes the deprecation message to always print

Mainly renaming service to graph and "schema" to service, which felt a little meh, but more in line with the name of the command

This should only include changes to some minor message changes in service:check

This supports a more modern name for the API key and adds deprecation messages for the (now legacy) ENGINE_API_KEY. Much in the same spirit as #1849, the goal is to modernize our documentation to no longer use the out-of-date name "engine". This also modifies tests to support the new version and adds a test that ensures an error is thrown when a user sets _both_ keys.

* Support APOLLO_KEY, deprecate ENGINE_API_KEY This supports a more modern name for the API key and adds deprecation messages for the (now legacy) ENGINE_API_KEY. Much in the same spirit as #1849, the goal is to modernize our documentation to no longer use the out-of-date name "engine". This also modifies tests to support the new version and adds a test that ensures an error is thrown when a user sets _both_ keys.

zionts changed the base branch from master to adam/20/3/no-graph-found-error March 25, 2020 05:17

zionts mentioned this pull request Mar 25, 2020

[GM-835] Change --tag references to --variant for commands apollographql/apollo#819

Merged

zionts requested a review from JakeDawkins March 25, 2020 05:43

zionts mentioned this pull request Mar 25, 2020

Update "no service found to link..." error #1847

Merged

4 tasks

JakeDawkins reviewed Mar 25, 2020

View reviewed changes

Adam Zionts added 21 commits March 25, 2020 11:19

Make newlines (optional commit)

fdb552f

Change references of Apollo Engine in flags

df140d8

These default flags were still referencing Apollo Engine instead of Graph Manager (even though some were hidden). Additionally, this updates the docs to be more accurate. Happy to take this commit into another PR if you'd like!

Give deprecation warning to --tag users

74bd14a

Rename errors util to sharedMessages

271982a

Move tag flag deprecation warning to sharedMessages

4549acf

Support the variant flag in service:check

373d364

Also remove an unused variable in service:list

Update service:delete to use variant flag

9658ff5

Also update description of the flags

Add support to service:download to use --variant

ed9bd15

Also simplify an import in apollo-language-server Also add a XXX comment to service:download to illustrate a problem

Change magical config tag to use variant

5a67bbb

Additionally, this updates the text output in the service:check command to consistently use graph@variant everywhere

Rename magical config.tag to config.variant

a2fa163

This is a find and replace all, and I verified every command still works after this (both with --tag and --variant). Will post screenshots of how everything looks in the PR :)

Update snapshots

fe93251

A previous commit added `@variant` everywhere, and these snapshot updates should be ONLY that.

Put all handling into the magical config.variant

598f2ae

Replace all flags.tag references to config.variant

272434a

Add variant to the client: commands

3813df4

All of the client commands share common flags, so no further changes are required. Also, all of them use config.variant, so yay! :)

Warn about deprecation at magical config.variant

9524e26

Remove the deprecation warning from specific callsites and rely on the deprecation warning appearing when the config.variant value is set

Changes word "tag" in some user-facing messages

606e18b

Some small tweaks in wording

5f44ed1

Mainly renaming service to graph and "schema" to service, which felt a little meh, but more in line with the name of the command

Update changelog

c47a12b

Updates snapshots to match new messaging

85e49e9

This should only include changes to some minor message changes in service:check

zionts force-pushed the adam/20/3/rename-tag-to-variant branch from 2497248 to 85e49e9 Compare March 25, 2020 18:19

zionts changed the base branch from adam/20/3/no-graph-found-error to master March 25, 2020 18:21

JakeDawkins merged commit 04bf842 into master Mar 25, 2020

JakeDawkins deleted the adam/20/3/rename-tag-to-variant branch March 25, 2020 18:35

zionts mentioned this pull request Mar 26, 2020

service:check doesn't always show the variant. #1665

Closed

This was referenced Mar 27, 2020

[GM-833] Add support for APOLLO_KEY; deprecate ENGINE_API_KEY #1851

Merged

Support engine.graphVariant and APOLLO_GRAPH_VARIANT. apollographql/apollo-server#3924

Merged

sachindshinde mentioned this pull request Apr 1, 2020

Remove "hidden: true" from variant flag in service:push #1865

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create --variant flag, deprecate --tag flag #1849

Create --variant flag, deprecate --tag flag #1849

zionts commented Mar 25, 2020 •

edited

Loading

JakeDawkins Mar 25, 2020

zionts Mar 25, 2020

JakeDawkins Mar 25, 2020

JakeDawkins Mar 25, 2020

Create --variant flag, deprecate --tag flag #1849

Create --variant flag, deprecate --tag flag #1849

Conversation

zionts commented Mar 25, 2020 • edited Loading

JakeDawkins Mar 25, 2020

Choose a reason for hiding this comment

zionts Mar 25, 2020

Choose a reason for hiding this comment

JakeDawkins Mar 25, 2020

Choose a reason for hiding this comment

JakeDawkins Mar 25, 2020

Choose a reason for hiding this comment

zionts commented Mar 25, 2020 •

edited

Loading