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

Don't apply enterprise state for oss builds. #3847

Merged
merged 8 commits into from
Aug 27, 2019
Merged

Conversation

pawanrawal
Copy link
Contributor

@pawanrawal pawanrawal commented Aug 22, 2019

Separate out all code which proposes the license and checks for its validity into two different files, license.go and license_ee.go. This ensures that license related operations are only done for !oss builds.


This change is Reviewable

@pawanrawal pawanrawal requested review from manishrjain and a team as code owners August 22, 2019 00:48
Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

✅ A review job has been created and sent to the PullRequest network.


@pawanrawal you can click here to see the review status or cancel the code review job.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

This looks good and I think using build tags to separate it is a good approach


Reviewed with ❤️ by PullRequest

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @pawanrawal)


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

		},
	}
	for {

No need to retry. Remove this func.


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

	}

	enabled := s.state.GetLicense().GetEnabled()

if !enabled { return }


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

	if enabled && !s.state.License.Enabled {
		// License was enabled earlier and has just now been disabled.
		glog.Infof("Enterprise license has expired and enterprise features would be disabled now. " +

100 chars.


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

	if enabled && !s.state.License.Enabled {
		// License was enabled earlier and has just now been disabled.
		glog.Infof("Enterprise license has expired and enterprise features would be disabled now. " +

Warningf


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

	expiry := time.Unix(s.state.License.ExpiryTs, 0)
	timeToExpire := expiry.Sub(time.Now())
	if enabled && timeToExpire > 0 && timeToExpire < humanize.Week {

Merge this func with the above.


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

	ctx, cancel := context.WithTimeout(context.Background(), time.Minute)
	defer cancel()
	if err := st.zero.applyEnterpriseLicense(ctx, bytes.NewReader(b)); err != nil {

st.zero.applyLicense.

Copy link
Contributor Author

@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.

Reviewable status: 1 of 5 files reviewed, 7 unresolved discussions (waiting on @manishrjain)


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

Previously, manishrjain (Manish R Jain) wrote…

No need to retry. Remove this func.

Can't remove this function it is called from raft.go which is common for oss and !oss builds. For !oss it makes a proposal and for oss it just returns nil. I have removed the retry loop.


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

Previously, manishrjain (Manish R Jain) wrote…

if !enabled { return }

Done.


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

Previously, manishrjain (Manish R Jain) wrote…

100 chars.

Done.


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

Previously, manishrjain (Manish R Jain) wrote…

Warningf

Done.


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

Previously, manishrjain (Manish R Jain) wrote…

Merge this func with the above.

Done.


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

Previously, manishrjain (Manish R Jain) wrote…

st.zero.applyLicense.

Done.


dgraph/cmd/zero/raft.go, line 528 at r1 (raw file):

}

// periodically checks the validity of the enterprise license and updates the membership state.

This function has been moved to license.go and license_ee.go.

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 4 of 4 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @manishrjain and @pawanrawal)


dgraph/cmd/zero/license_ee.go, line 82 at r2 (raw file):

			}

			expiry := time.Unix(license.GetExpiryTs(), 0)

Are these in UTC? We should normalize all times to UTC.

@pawanrawal pawanrawal merged commit 98880f8 into master Aug 27, 2019
@pawanrawal pawanrawal deleted the pawan/zero-ee-files branch August 27, 2019 02:46
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.

2 participants