apollographql / rover

The CLI for Apollo GraphOS
https://rover.apollo.dev
Other
409 stars 85 forks source link

Enable remote proxy binary downloads #2254

Open LongLiveCHIEF opened 2 weeks ago

LongLiveCHIEF commented 2 weeks ago

This PR adds the ability to specify a proxy/mirror remote for the binary installation methods for apollo rover. This functionality was added for the npm installer in #1713 and #1675, and is being added for the router binary installation script in https://github.com/apollographql/router/pull/6244

svc-apollo-docs commented 2 weeks ago

🚫 Docs Preview Denied

You must have approval from an Apollo team member to request a docs preview. If you are a team member, please comment !docs preview.

File Changes

4 new, 8 changed, 3 removed ``` + (developer-tools)/rover/commands/cloud.mdx + (developer-tools)/rover/configuring.mdx + (developer-tools)/rover/getting-started.mdx + (developer-tools)/rover/migration.mdx * (developer-tools)/rover/ci-cd.mdx * (developer-tools)/rover/index.mdx * (developer-tools)/rover/validating-client-operations.mdx * (developer-tools)/rover/commands/dev.mdx * (developer-tools)/rover/commands/graphs.mdx * (developer-tools)/rover/commands/persisted-queries.mdx * (developer-tools)/rover/commands/subgraphs.mdx * (developer-tools)/rover/commands/supergraphs.mdx - (developer-tools)/rover/configuring.md - (developer-tools)/rover/getting-started.md - (developer-tools)/rover/migration.md ```
LongLiveCHIEF commented 1 week ago

I sent @Meschreiber some extra details on Slack because I figured you may want to rename things. Wherever I put _BINARY_REMOTE the asset comes from GitHub. Wherever _DOWNLOAD_HOST is used, it's from orbiter endpoint.

Any of the above makes sense to me, but I also wonder... are the binaries the same? If so, why not just use the same endpoint for all installers, and get rid of the complexity of having env variables for multiple different endpoints?

On Fri, Nov 15, 2024, 1:17 PM Edward Huang @.***> wrote:

@.**** commented on this pull request.

On docs/source/getting-started.md https://github.com/apollographql/rover/pull/2254#discussion_r1844347270:

IIUC, the existing APOLLO_ROVER_DOWNLOAD_HOST env var sets the binary download URL for npm installation, while the new APOLLO_ROVER_BINARY_REMOTE would set the binary download URL for Bash and PowerShell. We need distinct env vars because the download URLs are usually different, but they serve similar purposes.

Given the above, it'd be ideal if the name of the new env var:

  1. Conveys it's a download URL for the specific Bash/PowerShell use case of downloading from a GitHub proxy/mirror
  2. Is consistent and/or uses the same namespacing as the existing env var, because they're for similar purposes.

For 2, we could use the existing prefix of APOLLO_ROVERDOWNLOAD*. For 1, we could append a suffix that conveys it's a GitHub proxy or remote mirror, something like:

  • APOLLO_ROVER_DOWNLOAD_GITHUB_PROXY
  • APOLLO_ROVER_DOWNLOAD_PROXY_HOST
  • APOLLO_ROVER_DOWNLOAD_GITHUB_HOST
  • APOLLO_ROVER_DOWNLOAD_GITHUB_MIRROR

thoughts @Meschreiber https://github.com/Meschreiber ?

— Reply to this email directly, view it on GitHub https://github.com/apollographql/rover/pull/2254#pullrequestreview-2439433127, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAZBLAMWHRT3SR7QE5LHXT32AZCDZAVCNFSM6AAAAABRLN6QROVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIMZZGQZTGMJSG4 . You are receiving this because you authored the thread.Message ID: @.***>

LongLiveCHIEF commented 1 week ago

Just a point to note, the host is a proxy if you set it, the same way. That remote is a proxy if you set it. I think if you make one say proxy, they should both say proxy, to limit the confusion.

I don't think it's good to think of them as exclusively used for bash/powershell variables, because they could very easily wind up being used for plugins or other installers.

It doesn't matter what the eventual URL is, that's entirely up to you guys.

Both _REMOTE and _HOST are ways for users to declare a proxy to use... and that proxy will be configured by the users networks to connect to whatever URL the installer (npm, bash, cargo, etc...) uses by default.

On Mon, Nov 18, 2024, 10:35 AM Maria Elisabeth Schreiber < @.***> wrote:

@.**** commented on this pull request.

On docs/source/getting-started.md https://github.com/apollographql/rover/pull/2254#discussion_r1846912088:

Great point, thanks for taking a look so quickly @shorgi https://github.com/shorgi !

I'm fine with any of the above. To decide, one question for @LongLiveCHIEF https://github.com/LongLiveCHIEF :

Does the download URL for Bash and PowerShell always have to be a GH repo URL? (Will it work with other binary URL sources?)

Besides that, I like the inclusion of PROXY to make clearer how it's different from DOWNLOAD_HOST.

— Reply to this email directly, view it on GitHub https://github.com/apollographql/rover/pull/2254#discussion_r1846912088, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAZBLANBUAAB7L2FYV3K2MD2BIJNZAVCNFSM6AAAAABRLN6QROVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDINBTGE2TAOBVHA . You are receiving this because you were mentioned.Message ID: @.***>

Meschreiber commented 1 week ago

Thanks @LongLiveCHIEF -- so is the keyword here GITHUB? Thoughts on APOLLO_ROVER_DOWNLOAD_GITHUB_HOST to complement APOLLO_ROVER_DOWNLOAD_HOST?

LongLiveCHIEF commented 1 week ago

Thanks @LongLiveCHIEF -- so is the keyword here GITHUB? Thoughts on APOLLO_ROVER_DOWNLOAD_GITHUB_HOST to complement APOLLO_ROVER_DOWNLOAD_HOST?

Yep, that would totally work. If that works for you, then I will rebase and update my PR's to use

APOLLO_ROVER_DOWNLOAD_GITHUB_HOST and APOLLO_ROUTER_DOWNLOAD_GITHUB_HOST

Meschreiber commented 1 week ago

@LongLiveCHIEF , works for me, thank you! 🙇‍♀️

LongLiveCHIEF commented 5 days ago

@Meschreiber @shorgi rebase complete on both PR's, using suggested syntax.

Meschreiber commented 2 days ago

@LongLiveCHIEF , sorry for the delay on getting back to this. It looks like there's some conflicts that need to be resolved?