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

Change error message in case of successful license application #5230

Merged
merged 5 commits into from
Apr 23, 2020

Conversation

all-seeing-code
Copy link
Contributor

@all-seeing-code all-seeing-code commented Apr 17, 2020

Before this code change in case of successful license application, http response would look like:
{ "errors": [ { "message": "Done", "extensions": { "code": "Success" } } ] } .
This is confusing since although the license has been correctly applied, it still shows the success in the error field. This PR changes the above response to:
{ "code": "Success", "message": "License applied." } to remove any confusion.

Fixes #GitHubIssue 4965
Fixes Dgraph-1135


This change is Reviewable

Docs Preview: Dgraph Preview

Copy link
Contributor

@jarifibrahim jarifibrahim left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @anurags92, @manishrjain, and @parasssh)


dgraph/cmd/zero/license_ee.go, line 133 at r1 (raw file):

		return
	}
	x.Check2(w.Write([]byte(`{"code": "Success", "message": "License applied."}`)))

w.Write call could return an error. If it fails, we will end up killing the server because Check2 contains log.Fatal. We should handle the error appropriately here.

Copy link
Contributor

@parasssh parasssh left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @anurags92, @manishrjain, and @parasssh)

a discussion (no related file):
:lgtm:


Copy link
Contributor

@parasssh parasssh left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @anurags92, @manishrjain, and @parasssh)

a discussion (no related file):
:lgtm:


Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jarifibrahim, @manishrjain, and @parasssh)

a discussion (no related file):

Previously, parasssh wrote…

:lgtm:

:lgtm: once the point Ibrahim raised is fixed. I added a bit of context on why Check2 is used sometimes in the codebase.



dgraph/cmd/zero/license_ee.go, line 133 at r1 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

w.Write call could return an error. If it fails, we will end up killing the server because Check2 contains log.Fatal. We should handle the error appropriately here.

Agree. Check2 is used sometimes but that's only to write to a string buffers because that's very unlikely to return an error when writing small strings.

But we don't have the same kind of control on the w writer. An error here should probably print a warning in the server logs at the least.

@all-seeing-code all-seeing-code merged commit dc8b80c into master Apr 23, 2020
danielmai pushed a commit that referenced this pull request Apr 24, 2020
Change error message in case of successful license application
danielmai pushed a commit that referenced this pull request Apr 24, 2020
Change error message in case of successful license application
all-seeing-code added a commit that referenced this pull request Jun 8, 2020
Change error message in case of successful license application

(cherry picked from commit dc8b80c)
all-seeing-code added a commit that referenced this pull request Jun 9, 2020
#5593)

Change error message in case of successful license application

(cherry picked from commit dc8b80c)
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 18, 2020
…h-io#5230)

Change error message in case of successful license application
@all-seeing-code all-seeing-code deleted the anurags92/EnterpriseLicenseMsg branch September 24, 2020 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Strange response when applying enterprise license
4 participants