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

ROVER-68 Ensure we always use correct Federation Version when Testing #1987

Merged
merged 5 commits into from
Jul 16, 2024

Conversation

jonathanrainer
Copy link
Contributor

@jonathanrainer jonathanrainer commented Jul 16, 2024

After the merge and rollback of bumping to supergraph v.2.8.3 (#1978 and #1982), there was some confusion as to why this wasn't picked up by CI until the bump had been merged. Turns out this is because Orbiter uses the main branch of the rover repo to know what the 'latest' version of the plugins should be, something I had not realised.

However, this leads to problems because the integration tests pick up the latest version from main not from the branch, so we end up in a situation where we cannot test changes to the latest_plugin_versions file until we merge to main. This PR fixes that for both the integration tests against Studio and the Smoke Tests, so we are always operating from the perspective of the state of the world the PR proposes, not the world as it actually is.

This should lead to us catching router and supergraph issues before they hit main because the Renovate PRs will fail rather than us finding out post-facto.

As an example see: https://circleci.com/gh/apollographql/rover/31971, where we emulated the bump to v2.8.3 of supergraph which broke our CI as was intended.

This encourages good practice from users
Now the smokes run the latest 3 versions including that specified
in the latest_plugin_versions JSON file
@jonathanrainer jonathanrainer marked this pull request as ready for review July 16, 2024 13:24
@jonathanrainer jonathanrainer requested a review from a team as a code owner July 16, 2024 13:24
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes to the file here vis-à-vis the name amd_macos vs arm_macos are to make it clearer what we're testing at each stage. Other than that we're just adding the environment variable we need in each case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Under normal circumstances I'd have implemented this as part of xtask but because this is going to run in GHA and run before we invoke the smoke tests we'd have to do a lot of messing around to get the binaries in the right places. As such it felt better to just implement it in Bash

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These types are just to make deserialising the latest_plugin_versions.json file easier

name: "Install `semver` cli"
- run: |
ls -al
JSON=$(source get_latest_x_versions.sh 3 apollographql router router latest-1 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: why use source over ./?

Copy link
Contributor Author

@jonathanrainer jonathanrainer Jul 16, 2024

Choose a reason for hiding this comment

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

So I tried using ./ but because you're running it in a subshell you have to use source otherwise the interpreter gets confused

supergraphs_path.push("supergraphs");
for dir_entry in fs::read_dir(supergraphs_path)? {
let path = dir_entry.unwrap().path();
if path.extension().unwrap() == "yaml" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Should we also handle .yml and case variations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes indeed, we run it through lowercase and then check with a vector so we should be grand

@jonathanrainer jonathanrainer merged commit 977a1f0 into main Jul 16, 2024
18 checks passed
@jonathanrainer jonathanrainer deleted the jr/task/ROVER-68 branch July 16, 2024 15:36
@jonathanrainer jonathanrainer added this to the v0.25.0 milestone Jul 16, 2024
@jonathanrainer jonathanrainer mentioned this pull request Jul 19, 2024
jonathanrainer added a commit that referenced this pull request Jul 22, 2024
# [0.25.0] - 2024-07-22

## 🚀 Features

- **Enable Retries For Transient Errors Connecting To Graphs/Subgraphs -
@jonathanrainer PR #1936**

This turns on retries at the HTTP level for connections to
graphs/subgraphs to minimize connection resets and cancellations. Also,
a new --subgraph-retries flag for rover dev lets you set the number of
retries allowed when trying to re-establish a connection.

- **Add `--graph-ref` flag to `rover dev` - @dotdat PR #1984**

Introduces subgraph mirroring to rover dev. Subgraph mirroring inherits
the subgraph routing URLs and schemas from an existing Studio graphref.
This makes it easy to spin up a locally running supergraph without
maintaining a supergraph config. [See
here](https://www.apollographql.com/docs/rover/commands/dev#starting-a-session-from-a-graphos-studio-variant)
for more information.

## 🐛 Fixes

- **Fixes issues related to passing filenames to `--output` -
@jonathanrainer PR #1996**

An issue was raised whereby previous versions of Rover supported passing
filenames to the `--output` flag but this was
broken in v0.24.0. This has now been fixed and the previous
functionality restored.

## 🛠 Maintenance

- **Expand Smoke Tests To Run On All Supported Platforms -
@jonathanrainer PR #1980**
- **Fix cron expression, so it runs only once per day - @jonathanrainer
PR #1986**
- **Ensure we always use the correct version of Federation when testing
- @jonathanrainer PR #1987**
- **Add manual Smoke test invocation and pin Windows to `npm@9` for
testing - @jonathanrainer PR #1989**
- **Update apollographql/router to v1.51.0 - @jonathanrainer PR #1988**
- **Update node.js packages - @jonathanrainer PR #1979**

Includes `@eslint/compat` to v1.1.1, `eslint` to v9.7.0, `node.js` to
v20.15.1, `npm` to v10.8.2 and `prettier` to v3.3.3

- **Make sure x86 Mac Tests use 'latest' supergraph plugin version -
@jonathanrainer PR #1990**
- **Make sure homebrew runs `brew update` when we use it -
@jonathanrainer PR #1993**



## 📚 Documentation

- **Adds `graph-ref` flag to dev subcommand docs - @jackonawalk PR
#1945**
- **Update schema proposals capabilities docs - @Meschreiber PR #1949**
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants