-
Notifications
You must be signed in to change notification settings - Fork 237
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
Keep requires directive in gateway schema #756
Conversation
@@ -710,16 +710,6 @@ test('Polling schemas (should properly regenerate the schema when a downstream s | |||
}) | |||
t.teardown(() => clock.uninstall()) | |||
const oldSchema = ` | |||
directive @extends on INTERFACE | OBJECT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't make sense for a service to have this defined in its schema. This is federation metadata.
@@ -804,16 +794,6 @@ test('Polling schemas (should properly regenerate the schema when a downstream s | |||
}) | |||
|
|||
const refreshedSchema = ` | |||
directive @extends on INTERFACE | OBJECT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't make sense for a service to have this defined in its schema. This is federation metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, good spot!
@jonnydgreen could you take a look as well? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Nice one!
Since it's a bugfix, I'm thinking we should backport this to v8 as well - what do you think @mcollina? |
Great idea, let's try the action. |
Fixes #729