Initial implementation of server licensing service #41026
Conversation
eliperelman
added
Team:Core
|
Pinging @elastic/kibana-platform |
|
Right now the current return value of setting up the server licensing service returns a type LicensingServiceSubject = BehaviorSubject<LicensingServiceSetup | null>;
const licensing: LicensingServiceSubject;
// ...
licensing.subscribe(license => {
if (!license) {
// Service is null until polled and started.
return;
}
const { status, message } = license.check('discovery', LICENSE_TYPE.gold);
// ...
}); |
eliperelman
force-pushed
the
licensing-service
branch
from
f78bdb4 to
0415e96
Compare
| from(this.fetchInfo(deps, clusterSource, pollingFrequency)).pipe( | ||
| tap(({ license, error, features }) => { | ||
| // If license is false, the license did not change and we don't need to push | ||
| // a new one |
There was a problem hiding this comment.
On line 91 it returns false if the license did change?
💔 Build Failed |
|
Overall, this is looking like how I would expect it to 👍 Per our discussion in the weekly, I would advise changing the interface exposed by the service to be a Observable of some interface rather than a class, and then you can implement that interface in the class. This makes testing and mocking easier for consumers. #39446 (comment) |
| private serializable: LicenseFeatureSerializer = service => ({ | ||
| name: this.name, | ||
| isAvailable: this.isAvailable, | ||
| isEnabled: this.isEnabled, |
There was a problem hiding this comment.
What's the difference between isAvailable and isEnabled?
There was a problem hiding this comment.
Not a clue, I'll see what I can find out. 😄
|
|
||
| private hasLicenseInfoChanged(newLicense: any) { | ||
| return ( | ||
| newLicense.mode !== this.license.mode || |
There was a problem hiding this comment.
What's mode? If this corresponds to the LICENSE_TYPE enum, then maybe type would make that clear?
There was a problem hiding this comment.
This was preexisting from x-pack legacy, so I'll see about making this clearer.
| private clusterSource: string | ||
| ) { | ||
| this._license = license; | ||
| this.license = license || {}; |
There was a problem hiding this comment.
I probably won't ever maintain this code, but if I did, I think I would be confused by the presence of this._license and this.license. I would be able to follow this code more easily if we only had one or the other. It looks like this._license is only used to assert whether there is a license or not. In that case, could we do this instead?
constructor(
license: any,
features: any,
private error: Error | null,
private clusterSource: string
) {
this.hasLicense = Boolean(license);
this.license = license || {};| return this.objectified; | ||
| } | ||
|
|
||
| feature(name: string) { |
There was a problem hiding this comment.
Minor nit, but I'd find this method easier to understand if there was a verb in the name, e.g. getFeature.
| * @internal | ||
| */ | ||
| constructor(rawConfig: LicensingConfigType, env: Env) { | ||
| this.enabled = rawConfig.enabled; |
There was a problem hiding this comment.
I see isEnabled used in some places and enabled in others. Should be standardize on one or the other? My preference would be isEnabled, because I'm used to seeing words like is used to identify booleans.
There was a problem hiding this comment.
Since this comes from configuration that we control, this is a good idea. Will revise.
💔 Build Failed |
eliperelman
removed
Feature:New Platform
Team:Core
Summary
Licensing service. This is currently a work in progress:
Fixes #18356.
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.For maintainers