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

graphql: support existing gqlschema nodes without xid #5457

Merged
merged 2 commits into from
May 19, 2020

Conversation

abhimanyusinghgaur
Copy link
Contributor

@abhimanyusinghgaur abhimanyusinghgaur commented May 18, 2020

This PR adds support for adding xid to existing gqlschema nodes as it was not taken into consideration in #5054.

Fixes #GRAPHQL-417.


This change is Reviewable

@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label May 18, 2020
Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

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

Is this a bugfix? If yes, then is it possible to add a testcase for this?

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @abhimanyusinghgaur and @MichaelJCompton)


graphql/admin/admin.go, line 472 at r1 (raw file):

	qry := &gql.GraphQuery{
		Attr: existingSchemaVar,

It would be helpful to write down the final query in comments that would be built here.

Copy link
Contributor Author

@abhimanyusinghgaur abhimanyusinghgaur left a comment

Choose a reason for hiding this comment

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

Added a testcase which checks that the GqlSchema node always has dgraph.graphql.xid with value dgraph.graphql.schema after graphql layer has started properly.

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @MichaelJCompton and @pawanrawal)


graphql/admin/admin.go, line 472 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

It would be helpful to write down the final query in comments that would be built here.

Done.

Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @MichaelJCompton)

Copy link
Contributor

@MichaelJCompton MichaelJCompton left a comment

Choose a reason for hiding this comment

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

Yeah, this kind of thing might be hard to test in our testing setup ... ideally, we'd have some way where we could roll through an update from one version to the next. So I guess you could have a test with a 20.03.1 version, then restart on a 20.03.3 and see if it all updates smoothly. I don't think it's worth building that out just for this case. But I think it's a test we should have in general that runs from a 20.03.x to a 20.03.x+1 cause that gives us an assurance that a customer can upgrade smoothly.

Also, merging this, means we are committing to fixing this #5235 asap.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @MichaelJCompton)

@abhimanyusinghgaur abhimanyusinghgaur merged commit 85bd30c into master May 19, 2020
@abhimanyusinghgaur abhimanyusinghgaur deleted the abhimanyu/migrate-schema branch May 19, 2020 06:15
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 18, 2020
This PR adds support for adding xid to existing gqlschema nodes as it was not taken into consideration in dgraph-io#5054.

Fixes #GRAPHQL-417.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/graphql Issues related to GraphQL support on Dgraph.
Development

Successfully merging this pull request may close these issues.

3 participants