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-135 Make paths in supergraph.yaml resolve relative to the location of the file, not the current working directory of Rover #2119

Merged
merged 7 commits into from
Sep 10, 2024

Conversation

jonathanrainer
Copy link
Contributor

@jonathanrainer jonathanrainer commented Sep 10, 2024

As per the title, to support integration with the Language Server we need to remove a 'quirk' of Rover whereby if you specify a file in the supergraph.yaml with a relative path, that path is resolved relative to the location that rover is running in at the time. This doesn't work in that context so instead we are changing it to resolve relative to the location of the supergraph.yaml file itself.

I've added a unit test for this and run my own tests and it all seems to work ok.

In addition I've fixed a test that has been broken for a long time, a more detailed write up on this test will follow

Successful smoke test run: https://github.com/apollographql/rover/actions/runs/10795662532/job/29943305762

Still needs tests and will need a federation-rs PR merging
to get it all aligned.
It appears these particular tests were relying on specific Windows
line breaks etc. This is very error-prone, and in reality unnecessary
as we can match on a smaller subset of the query itself and still
get sensible answers.
@jonathanrainer jonathanrainer added feature 🎉 new commands, flags, functionality, and improved error messages BREAKING ❗ a PR that represents a breaking change labels Sep 10, 2024
@jonathanrainer jonathanrainer requested a review from a team as a code owner September 10, 2024 15:25
@dotdat dotdat merged commit 1ed1a27 into composition-integration Sep 10, 2024
34 checks passed
@dotdat dotdat deleted the jr/story/ROVER-135 branch September 10, 2024 16:26
dotdat pushed a commit that referenced this pull request Sep 16, 2024
…ion of the file, not the current working directory of Rover (#2119)

As per the title, to support integration with the Language Server we
need to remove a 'quirk' of Rover whereby if you specify a file in the
supergraph.yaml with a relative path, that path is resolved relative to
the location that rover is running in at the time. This doesn't work in
that context so instead we are changing it to resolve relative to the
location of the supergraph.yaml file itself.

I've added a unit test for this and run my own tests and it all seems to
work ok.

In addition I've fixed a test that has been broken for a long time, a
more detailed write up on this test will follow

Successful smoke test run:
https://github.com/apollographql/rover/actions/runs/10795662532/job/29943305762
aaronArinder pushed a commit that referenced this pull request Sep 18, 2024
…ion of the file, not the current working directory of Rover (#2119)

As per the title, to support integration with the Language Server we
need to remove a 'quirk' of Rover whereby if you specify a file in the
supergraph.yaml with a relative path, that path is resolved relative to
the location that rover is running in at the time. This doesn't work in
that context so instead we are changing it to resolve relative to the
location of the supergraph.yaml file itself.

I've added a unit test for this and run my own tests and it all seems to
work ok.

In addition I've fixed a test that has been broken for a long time, a
more detailed write up on this test will follow

Successful smoke test run:
https://github.com/apollographql/rover/actions/runs/10795662532/job/29943305762
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BREAKING ❗ a PR that represents a breaking change feature 🎉 new commands, flags, functionality, and improved error messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants