-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
graphql: duplicate /health in GraphQL /admin #4768
Conversation
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.
Reviewed 6 of 9 files at r1, 3 of 3 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @manishrjain and @MichaelJCompton)
graphql/admin/admin.go, line 57 at r2 (raw file):
} """Node state is the state of an individual node int he Dgraph cluster """
in the
graphql/e2e/common/common.go, line 598 at r2 (raw file):
} func hasCurrentGraphQLSchema(url string) (bool, error) {
Wondering if we could just have one return parameter given how this is used.
graphql/resolve/resolver.go, line 497 at r2 (raw file):
// injectAliasCompletion takes a result with names as per the type names and swaps those for // any aliases specified in the query before apply cf.
I don't completely understand this. Do we have a test which showcases this? Is it just about returning the query result along with any aliases that are specified?
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.
Reviewable status: 8 of 9 files reviewed, 4 unresolved discussions (waiting on @golangcibot, @manishrjain, and @pawanrawal)
graphql/admin/admin.go, line 57 at r2 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
in the
Done.
graphql/e2e/common/admin.go, line 276 at r1 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
G107: Potential HTTP request made with variable url (from
gosec
)
Done.
graphql/e2e/common/common.go, line 598 at r2 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Wondering if we could just have one return parameter given how this is used.
nah, we differentiate between error, false - there's no schema and true - there is a schema
graphql/resolve/resolver.go, line 497 at r2 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
I don't completely understand this. Do we have a test which showcases this? Is it just about returning the query result along with any aliases that are specified?
Yeah, the processing that goes through Dgraph handles aliases by injecting them into the Dgraph query, but the admin functions don't have that. So they all need post processing to handle the aliases.
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.
Reviewable status: 8 of 9 files reviewed, 4 unresolved discussions (waiting on @golangcibot, @manishrjain, and @pawanrawal)
Note that the intention here is to keep /health, not replace it.
This change is
Docs Preview: