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

Licensing: allow multiple registration of the same feature #76272

Merged
merged 1 commit into from
Sep 1, 2020

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Aug 31, 2020

Summary

Adapt the FeatureUsage service to accept multiple registrations of the same feature, as long as the licenseType is the same.

This is driven by the fact that features can now be registered from the client-side, so every refresh or new client can call this API, resulting in multiple calls.

Checklist

@pgayvallet pgayvallet added release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.10.0 labels Aug 31, 2020
Comment on lines +46 to +53
const registered = this.lastUsages.get(featureName);
if (registered) {
if (registered.licenseType !== licenseType) {
throw new Error(
`Feature '${featureName}' has already been registered with another license type. (current: ${registered.licenseType}, new: ${licenseType})`
);
}
} else {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@restrry This is the easiest fix for the problem, and answer the need.

Another option would be to instead adapt the client-side service to fetch the list of already registered features from the server-side during setup and to not perform calls to the server when trying to register an already existing feature. This is just a little trickier if we want to keep the client-side service synchronous

export class FeatureUsageService {
public setup({ http }: SetupDeps): FeatureUsageServiceSetup {
return {
register: async (featureName, licenseType) => {
await http.post('/internal/licensing/feature_usage/register', {
body: JSON.stringify({
featureName,
licenseType,
}),
});
},
};
}

as it would mean creating the fetch promise during setup and await for its result in every call to register. But nothing that can't be done. Do you think this would be better?

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the solution you have in PR. It's simpler and less likely to break IMO.

@Dosant
Copy link
Contributor

Dosant commented Aug 31, 2020

Other option could be to allow multiple registerFeature calls from client side, but leave validation for server side api?
Could we check inside route handler if featureName already registered? If yes, then respond 200, if no, then register and respond 200.

@pgayvallet
Copy link
Contributor Author

Yea, that would be another possibility

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@pgayvallet pgayvallet marked this pull request as ready for review August 31, 2020 14:59
@pgayvallet pgayvallet requested a review from a team as a code owner August 31, 2020 14:59
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@Dosant Dosant mentioned this pull request Aug 31, 2020
2 tasks
Comment on lines +46 to +53
const registered = this.lastUsages.get(featureName);
if (registered) {
if (registered.licenseType !== licenseType) {
throw new Error(
`Feature '${featureName}' has already been registered with another license type. (current: ${registered.licenseType}, new: ${licenseType})`
);
}
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the solution you have in PR. It's simpler and less likely to break IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants