-
Notifications
You must be signed in to change notification settings - Fork 275
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
promote include_subgraph_errors plugin #1776
Conversation
It has been experimental for a while and is now official. fixes: #1773
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! one comment on CL.
I was too late to comment on this. I'm not massively keen on the name of the plugin, and feel that eventually it could be part of a larger dev plugin in the config. I guess if and when that happens we can just deprecate this one. |
@BrynCooke Let's definitely chat about it. I agree this will need to evolve, though many production users have actually taken the effort to make their subgraph errors meaningful and safe and want them exposed, so they'll want a way to do that. |
@@ -189,7 +189,7 @@ impl Configuration { | |||
Value::Bool(true), | |||
); | |||
self.plugins.plugins.as_mut().unwrap().insert( | |||
"experimental.include_subgraph_errors".to_string(), | |||
"apollo.include_subgraph_errors".to_string(), |
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.
Hmmm I think now it's an apollo one, it's no longer located in self.plugins
but in self.apollo_plugins
isn't it ?
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.
I thought it was. User plugins are separate, but I thought experimental and apollo lived together
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.
I think it is in the wrong place, but the processing of mandatory plugins positions it correctly. We should still fix this at some point.
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.
see: #1777
It has been experimental for a while and is now official.
fixes: #1773