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: warn on unroutable --routing-urls #1477

Closed
patrick91 opened this issue Jan 19, 2023 · 1 comment · Fixed by #1565 or #1581
Closed

feat: warn on unroutable --routing-urls #1477

patrick91 opened this issue Jan 19, 2023 · 1 comment · Fixed by #1565 or #1581
Assignees
Labels
feature 🎉 new commands, flags, functionality, and improved error messages triage issues and PRs that need to be triaged
Milestone

Comments

@patrick91
Copy link
Contributor

patrick91 commented Jan 19, 2023

Hi there :)

I've noticed a couple of instances where we had either:

rover subgraph publish my-supergraph@my-variant \
  --schema "./accounts/schema.graphql" \
  --name accounts \
  --routing-url "my-running-subgraph.com/api" # note the missing scheme here

or

rover subgraph publish my-supergraph@my-variant \
  --schema "./accounts/schema.graphql" \
  --name accounts \
  --routing-url $NON_EXISTENT_VAR

leading to issues in our Cloud Router due to wrong or empty URLs. In our codebase we mention:

    /// Url of a running subgraph that a supergraph can route operations to
    /// (often a deployed subgraph). May be left empty ("") or a placeholder url
    /// if not running a gateway or router in managed federation mode

I wonder if we could improve this. Passing the flag with an empty URL feels a bit strange to me and it might have been caused by a misconfiguration (like in the second case there).

I don't have all the context so maybe being allowed to at least use a placeholder url is fine, but maybe we can validate the format[1]? so if you need a placeholder you can do http://abc?

Does this all make sense? 😊

[1] we should also have this check on the router/cloud-router to be fair

@patrick91 patrick91 added feature 🎉 new commands, flags, functionality, and improved error messages triage issues and PRs that need to be triaged labels Jan 19, 2023
@EverlastingBugstopper
Copy link
Contributor

EverlastingBugstopper commented Jan 19, 2023

This is a great point @patrick91 and one that I was tripped up on when we first introduced this command. I think there's a lot we can do here to make the experience better, but we'll need to be sure that we don't break existing workflows that may depend on invalid or placeholder values.

Here's what I'm thinking:

When a --routing-url is passed to rover subgraph publish:

  • In the case where a human is manually doing a publish (this can be determined by checking for a TTY) then Rover prints a warning that the URL is invalid with a prompt ([y/N]) to continue with the publish.

  • In the case where CI or a script is doing the publish (check for a TTY) then Rover prints a warning saying the routing url is invalid, but still allow the publish to go through. We might also want to make the warning include a notice that this behavior may break in future versions of Rover if the --allow-invalid-routing-url flag is not passed.

  • In both scenarios (and in future versions of Rover), passing the --allow-invalid-routing-url flag should disable the warnings entirely and allow the invalid routing url to be used.

When a --routing-url is not passed to rover subgraph publish:

  • Perform a GraphQL query against the platform API to get the routing url for that subgraph
  • If that URL is invalid, follow the same error/warn procedure as described above

@EverlastingBugstopper EverlastingBugstopper changed the title UX consideration for--routing-url param in subgraph publish feat: warn on unroutable --routing-urls Mar 29, 2023
@EverlastingBugstopper EverlastingBugstopper added this to the vNext milestone Mar 29, 2023
EverlastingBugstopper added a commit that referenced this issue Apr 17, 2023
Supersedes #1543 (took a different approach, didn't care to destroy /
overwrite the history there)

Fixes #1477

This feature addition adds URL validation to the rover subgraph publish
`--routing-url` flag. It also introduces a dependent flag to bypass
warnings/prompts `--allow-invalid-routing-url`.

Taken from @EverlastingBugstopper 's
#1477 (comment):
When a `--routing-url` is passed to rover subgraph publish:

In the case where a human is manually doing a publish (this can be
determined by checking for a TTY) then Rover prints a warning that the
URL is invalid with a prompt (`[y/N]`) to continue with the publish.

In the case where CI or a script is doing the publish (check for a TTY)
then Rover prints a warning saying the routing url is invalid, but still
allow the publish to go through. We might also want to make the warning
include a notice that this behavior may break in future versions of
Rover if the `--allow-invalid-routing-url` flag is not passed.

In both scenarios (and in future versions of Rover), passing the
`--allow-invalid-routing-url` flag should disable the warnings entirely
and allow the invalid routing url to be used.

When a `--routing-url` is not passed to rover subgraph publish:

Perform a GraphQL query against the platform API to get the routing url
for that subgraph
If that URL is invalid, follow the same error/warn procedure as
described above

<!--
First, 🌠 thank you 🌠 for taking the time to consider a contribution to
Apollo!

Here are some important details to follow:

* ⏰ Your time is important
To save your precious time, if the contribution you are making will
take more than an hour, please make sure it has been discussed in an
        issue first. This is especially true for feature requests!

* 💡 Features
Feature requests can be created and discussed within a GitHub Issue.
Be sure to search for existing feature requests (and related issues!)
prior to opening a new request. If an existing issue covers the need,
please upvote that issue by using the 👍 emote, rather than opening a
        new issue.

* 🕷 Bug fixes
These can be created and discussed in this repository. When fixing a
bug,
please _try_ to add a test which verifies the fix. If you cannot, you
should
still submit the PR but we may still ask you (and help you!) to create a
test.

* 📖 Contribution guidelines
Follow https://github.com/apollographql/rover/blob/HEAD/CONTRIBUTING.md
when submitting a pull request. Make sure existing tests still pass, and
add
        tests for all new behavior.

* ✏️ Explain your pull request
Describe the big picture of your changes here to communicate to what
        your pull request is meant to accomplish. Provide 🔗 links 🔗 to
        associated issues!

We hope you will find this to be a positive experience! Open source
contribution can be intimidating and we hope to alleviate that pain as
much
as possible. Without following these guidelines, you may be missing
context
that can help you succeed with your contribution, which is why we
encourage
discussion first. Ultimately, there is no guarantee that we will be able
to
merge your pull-request, but by following these guidelines we can try to
avoid disappointment.

-->

---------

Co-authored-by: Avery Harnish <avery@apollographql.com>
EverlastingBugstopper added a commit that referenced this issue Apr 19, 2023
# [0.14.0] - 2023-04-19

> Important: 1 potentially breaking changes below, indicated by **❗
BREAKING ❗**

## ❗ BREAKING ❗

- **`rover config whoami` outputs to `stdout` instead of `stderr` and
using `--format json` includes more information than success or failure
- @scombat, #1560 fixes #1380**

When running `rover config whoami`, the output will print to `stdout`
instead of `stderr`. This may break scripts that relied on parsing the
output from `stderr`. The good news is that these scripts should be
easier to write because passing `--format json` to `rover config whoami`
will print structured output that can be parsed with a tool like
[`jq`](https://stedolan.github.io/jq/tutorial/).

## 🚀 Features

- **ALlow custom headers when running introspection with `rover
supergraph compose` - @dbanty, #1574 fixes #615**

A new field is available in `supergraph.yaml` files that allows sending
headers along with introspection. This value also supports environment
variable interpolation for sensitive values like authentication tokens.

- **Print a wanring when attempting to publish a subgraph with an
invalid routing URL - @trevor-scheer, #1543 fixes #1477**

When running `rover subgraph publish`, if the `--routing-url` you
specify or the routing URL stored in GraphOS is unroutable, a warning
will be printed. If you are not in CI, you will need to manually confirm
the publish to continue. You can dismiss the warning by passing
`--allow-invalid-routing-url`.

  **Note:** This warning will become a hard error in the future.

## 🐛 Fixes

- **Spawn a thread to avoid a rare deadlock in `rover dev` -
@EverlastingBugstopper, #1548 fixes #1544**

## 🛠 Maintenance

- **Updates dependencies - @EverlastingBugstopper, #1562**

  `apollo-parser` 0.4 -> 0.5
  `git2` 0.16 -> 0.17
  `opener` 0.5 -> 0.6
  `predicates` 2 -> 3
  `serial_test` 1 -> 2
  `toml` 0.5 -> 0.7
  ~`crossterm`~

- **Use Apple Silicon in CI - @EverlastingBugstopper, #1557 fixes
#1555**

There should be no user facing change here, we just run builds in CI
much faster.

## 📚 Documentation

- **Adds Apollo CLI migration guide to Rover docs - @StephenBarlow,
#1568**

The (deprecated) Apollo CLI documentation and the migration guide for
Rover now live in Rover's docset.

- **Cleans up nomenclature and links in Rover docs - @StephenBarlow,
#1571 and #1573**

Rover's documentation has been updated to refer to the [new GraphOS
documentation](https://www.apollographql.com/docs/graphos) along with
updating some terminology.

- **Mention community-maintained installation methods - @dbanty, #1542**

Rover's documentation now mentions the unofficial installation methods
`nix` and `brew`.
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 triage issues and PRs that need to be triaged
Projects
None yet
3 participants