Skip to content

Future-proof installer script and re-introduce target triplets#1433

Merged
abernix merged 8 commits intomainfrom
abernix/restage-triples-change
Aug 26, 2022
Merged

Future-proof installer script and re-introduce target triplets#1433
abernix merged 8 commits intomainfrom
abernix/restage-triples-change

Conversation

@abernix
Copy link
Member

@abernix abernix commented Jul 25, 2022

This is meant to reintroduce the Target Triplet pattern which was first introduced in #1393 and the #1405 follow-up, which were intentionally reverted prior to the 0.12.0 release because the existing installer script wouldn't correctly continue to pin the version in a way that wouldn't break existing users had we deployed that technique. The reason that was challenging is that we hadn't fixed the underlying issue.

This PR has meaningful individual commits that are worth reviewing. The first two are literally cherry-picks from previous iterations, followed by other commits which solidify more durable behavior. The previous installer should continue to work just fine, but as we're doing a major version bump, I suspect even if that doesn't work out we'll be okay for the long-term.

This also relates to (and still blocks) #1397.

There's some TODO items that we should account for before re-landing this, in particular, adopting the same pattern that Rover used which was more defensive in the way that it preserved the installation script within the Git tag of the release, making it possible for the Orbiter to reliably redirect a user to a immutable version of the install script that was current as of the time of the release. (Rather than the method of today where the current release installer tries willingly to install any past release (which will fail after the target triplets change.)

We originally cargo culted the Rover installer when making the Router installer exist, but a number of nuanced differences have emerged in it. This updates the installer in a variety of ways to align with that and provide a more durable installer experience:

  • The version of the installer is now embedded in the script itself as PACKAGE_VERSION. This pushes the burden of determining what latest is to [Orbiter] (our AWS Lambda function which runs in Netlify). It also allows the file that is in the commit that is in the Git release tag to be the installer that knows how to install that version best.

    In other words, this gives us freedom to make material changes to the way our installer process without relying on (the current) behavior which leaves every version's installer needing be able to install every version of the Router which is problematic in a variety of ways. For example, if you want to change the filenames in the release (or the architecture negotiation), which brings me to my next point.

  • We no longer use positional parameters to the install script to indicate the version we're going to solve. The previous method wasn't durable for the reason listed above (and the reason we couldn't land what should have been a relatively simple change to the file formats of our release artifacts). It is Orbiter's responsibility to point to the correct version of the installer for a particular version of the Router and pinning a version should be done via the Orbiter URL itself. Behind the scenes that redirects to the Git tag of the release. This also allows us to track our download counts through a centralized point. See this code for more details.

    The way to install the latest version of the Router — always — remains as it was:

    curl -sSL https://router.apollo.dev/download/nix/latest | sh

    The way to install a specific version of the Router also remains as it was:

    curl -sSL https://router.apollo.dev/download/nix/v0.15.1 | sh
  • Introduces more specific filenames which use a standardized representation of the architecture - "target triples". Not only is this the pattern that we use on Rover which is meaningfully important for reducing cognitive overhead, but this it is also a more common representation in the general sense and necessary to support a full range of binaries. Since Rover actually does the downloading, this also simplifies how it needs to think about our release artifacts, at least in the near term. Also there is now a v in front of the version in the tarball name instead of being yet again subtly different than Rover.

  • Adds support for wget, in addition to curl for downloading the release artifact from GitHub. Containers often don't have either and sometimes just one or the other. We literally have the code written already to negotiate between the two, so supporting both seems fine. It's also just one less diff between the files.

  • Adds a very necessary assertion that mktemp exists on the system. Again, one less difference between what Rover does and Router and meaningful UX for installers.

Fixes #1385

@Geal Geal added the api/1.0 label Aug 12, 2022
Geoffroy Couprie and others added 5 commits August 25, 2022 14:14
code extracted from rover's xtask command
…1405)

This follows-up #1393 with the nececessary changes to the install script
which allows the installer to locate (and extract) the right binaries during
the `router.apollo.dev` installation bits.
We originally cargo culted the Rover installer when making the Router
installer exist, but a number of nuanced differences have emerged in it.

This updates the installer in a variety of ways:

- The version of the installer is now embeeded in the script itself as
  `PACKAGE_VERSION`.  This pushes the burden of determining what `latest` is
  to [Orbiter] (our AWS Lambda function which runs in Netlify).  It also
  allows the file that is in the commit that is in the Git release tag to be
  the installer that knows how to install that version best.

  In other words, this gives us freedom to make material changes to the way
  our installer process without relying on (the current) behavior which leaves
  every version's installer needing be able to install every version of the
  Router which is problematic in a variety of ways.  For example, if you
  want to change the filenames in the release (or the architecture
  negotiation), which brings me to my next point.

- Introduces more specific filenames which use a standardized representation
  of the architecture - "target triples". Not only is this the pattern that
  we use on Rover which is meaningfully important for reducing cognivite
  overhead, but this it is also a more common representation in the general
  sense and necessary to support a full range of binaries.  Since Rover
  actually does the downloading, this also simplfies how it needs to think
  about our release artifacts, at least in the near term.

- Adds support for `wget`, in addition to `curl` for downloading the release
  artifact from GitHub.  Containers often don't have either and sometimes
  just one or the other.  We literally have the code written already to
  negotiate between the two, so supporting both seems fine.  It's also just
  one less diff between the files.

- Adds a very necessary assertion that `mktemp` exists on the system.
  Again, one less difference between what Rover does and Router and
  meaingful UX for installers.

[Orbiter]: https://github.com/apollograpqhl/orbiter/
@abernix abernix force-pushed the abernix/restage-triples-change branch from da50c78 to 63eb1af Compare August 25, 2022 14:15
@abernix abernix marked this pull request as ready for review August 25, 2022 14:17
The fact that we introduced such subtle changes to code that was proven to
both work and do exactly what we wanted has been really frustrating to chase
down.  Cargo culting is one thing, but to cargo cult and just make small
nuanced and arguably unnecessary changes that cannot be easily reconciled by
humans or tooling is extra cognitive overhead that we can do without.

There's certainly the argument that we could have written a library, but the
rule of threes is still a pretty reasonable thing to do before creating
abstractions.   In the general sense, it should be trivial to let two copies
of a similar script exist and stay relatively aligned when their purposes
are identical.
@abernix abernix changed the title Reintroduce target triplets Future-proof installer script and re-introduce target triplets Aug 25, 2022
Copy link
Contributor

@SimonSapin SimonSapin left a comment

Choose a reason for hiding this comment

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

I find this to be an uncomfortable amount of shell script code, but oh well

Copy link
Contributor

@garypen garypen left a comment

Choose a reason for hiding this comment

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

Looks good. shellcheck complained about the style thing, so I thought I'd mention it. Feel free to ignore since that was already there.

Comment on lines +61 to 62
downloader "$_url" "$_file"
if [ $? != 0 ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: this is considered to be better style (SC2181)

Suggested change
downloader "$_url" "$_file"
if [ $? != 0 ]; then
if ! downloader "$_url" "$_file"; then


# Router version defined in apollo-router's Cargo.toml
# Note: Change this line manually during the release steps.
PACKAGE_VERSION="v0.16.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

could this be overridden by an environment variable or an argument?

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean, yes, but again, same comment as #1433 (comment), the point is that is not durable because the package installer might change over time based on the version. It is very intentional that it does not allow installing different versions. 😸

If you want a different version, you change the version in the curl install URL path itself. That lets us track the actual invocations of the installer and change the installer over time with the versions.

BINARY_DOWNLOAD_PREFIX="https://github.com/apollographql/router/releases/download"

# Router version defined in apollo-router's Cargo.toml
# Note: Change this line manually during the release steps.
Copy link
Member Author

Choose a reason for hiding this comment

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

A future step should automate bumping this, in the same way that it's already automated on rover.


# Router version defined in apollo-router's Cargo.toml
# Note: Change this line manually during the release steps.
PACKAGE_VERSION="v0.16.0"
Copy link
Member Author

Choose a reason for hiding this comment

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

I mean, yes, but again, same comment as #1433 (comment), the point is that is not durable because the package installer might change over time based on the version. It is very intentional that it does not allow installing different versions. 😸

If you want a different version, you change the version in the curl install URL path itself. That lets us track the actual invocations of the installer and change the installer over time with the versions.

@abernix abernix merged commit f48ccc7 into main Aug 26, 2022
@abernix abernix deleted the abernix/restage-triples-change branch August 26, 2022 10:14
abernix added a commit that referenced this pull request Aug 26, 2022
Following up on #1433 which
re-introduced target triplets and the installer to support it, this utilizes
the installer in our Docker build steps.

This consists of:

- Switching our direct downloading of releases to go through our unified
  installer endpoint.
- Durably uses the installer script to download the right architecture for a
  particular release using the logic in the installer for that release.
- There is now a default `ROUTER_RELEASE` value of `latest` on the images,
  though the argument still works (and is used by our releasing pipeline)
- Simplifies some logic (e.g., transforming version numbers to remove `v`).
- Switching from `alpine` to `debian` since our installer doesn't support
  `alpine` and fails.  This is _not_ a material change to the outcome
  because we were only using the alpine image to download a tarball, not for
  runtime.  (Fwiw, it's worth, this change wouldn't have been necessary, but
  our installer runs `router --version` at the end which fails on Alpine.)
  Conceviably our Oribiter endpoint can just redirect to the tarballs with a
  small change which it already does for Rover, but this was quicker.
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.

pre-built binaries should include fully qualified target

4 participants