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

[DNM] ui build: split admin ui build based on license #19850

Closed
wants to merge 2 commits into from

Conversation

couchand
Copy link
Contributor

@couchand couchand commented Nov 6, 2017

This change plumbs the OSS/CCL build choice through to the admin UI build, to enable development of upcoming UI features covered by the CCL. CCL code is expected to be encapsulated by hook components, each of which will have a no-op version present in the OSS build.

I've also included a demonstration of how to implement a component under this system, but that is not intended to be merged. After people have taken a look at this, I'll move that information into documentation.

@couchand couchand requested review from a team November 6, 2017 21:25
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@cuongdo
Copy link
Contributor

cuongdo commented Nov 6, 2017

What is the result when a user hits /ccl_only_admin_ui_link and they are using OSS build?

Copy link
Contributor

@benesch benesch left a comment

Choose a reason for hiding this comment

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

This approach LGTM provided everyone is comfortable with this method of CCL'ing. Nice work!

@benesch
Copy link
Contributor

benesch commented Nov 6, 2017

@cuongdo, we plan to stub out each top-level CCL component. So if you have an entire page under CCL, you'd presumably provide an OSS stub page that read "this page is only available in CCL binaries".

@cuongdo
Copy link
Contributor

cuongdo commented Nov 6, 2017

@benesch That sounds generally good. There should be some way of converting users to CCL builds, such as a link to https://www.cockroachlabs.com/product/cockroachdb-enterprise/

cc @dianasaur323 who will likely have better ideas

@couchand
Copy link
Contributor Author

couchand commented Nov 6, 2017

@cuongdo This change doesn't really get into routing (or specifics of how any fallback/alternative implementation would be handled). This mechanism is very general - the OSS build would implement a stub with the fallback behavior.

I've made suggestions on what would happen with the routing of the first CCL changes over on #19643. The implementation might be something like:

// ccl/src/whatever.tsx
export function addRoutes() {
  return <Route path="/whatever" component={Implementation} />;
}

// src/whatever.tsx
export function addRoutes() {
  return <Redirect path="/whatever" to="/fallback" />;
}

If we start to see common patterns we can refactor things later.

@bdarnell
Copy link
Contributor

bdarnell commented Nov 7, 2017

LGTM


Review status: 0 of 12 files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

This change plumbs the OSS/CCL build choice through to the admin UI
build, to enable development of upcoming UI features covered by the CCL.
CCL code is expected to be encapsulated by hook components, each of
which will have a no-op version present in the OSS build.
@couchand
Copy link
Contributor Author

One significant requirement is not covered by this: handling clusters running the CCL build but without a valid license, which need to fall back to the OSS version of components.

@benesch
Copy link
Contributor

benesch commented Nov 13, 2017

which need to fall back to the OSS version of components.

At least some of them should display "enterprise license expired", no? You can just drop a if (license.isExpired()) return <LicenseExpired>; in the render method of each CCL entrypoint component. Somewhat inelegant, but equivalent to what we do on the Go side, and probably tractable as long as the number of CCL entrypoints is small.

@couchand
Copy link
Contributor Author

There are two cases: one is an expired license, but another is someone who downloaded our compiled binaries but just wants to use the OSS features. It would be a little strange for them to see "license expired", or indeed anything related to enterprise licensing, in that case.

@benesch
Copy link
Contributor

benesch commented Nov 13, 2017

Agreed. You could check for both cases in the CCL entrypoints, though.

@couchand
Copy link
Contributor Author

couchand commented Nov 13, 2017

It just means we need to actually bundle the OSS versions, too, and allow the CCL components to reference the OSS ones. So I think we'll need to rethink the structure.

@benesch
Copy link
Contributor

benesch commented Nov 13, 2017

That should happen automatically if the CCL plugin looks like this:

if (!license.exists())
    return <OSSThingy>
else if (!license.valid())
    return <LicenseExpiredThingy>
else
    return <CCLThingy>

@couchand
Copy link
Contributor Author

Yes, but how exactly do you import the OSS one from the CCL one? The configuration in this PR won't allow it, AFAICT.

@dianasaur323
Copy link
Contributor

dianasaur323 commented Nov 13, 2017 via email

@couchand
Copy link
Contributor Author

Closing in favor of #20051

@couchand couchand closed this Nov 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants