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.

Sign up for GitHub

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

Jump to bottom

[Maps] Always check license at plugin startup #87873

Merged
merged 8 commits into from
Jan 19, 2021
Merged

[Maps] Always check license at plugin startup #87873

merged 8 commits into from
Jan 19, 2021

Conversation

thomasneirynck
Copy link
Contributor

Summary

Closes #85863

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@thomasneirynck thomasneirynck added the [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation label Jan 11, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis (Team:Geo)

@thomasneirynck thomasneirynck added bug Fixes for quality problems that affect the customer experience v7.11.1 v7.12.0 v8.0.0 release_note:fix and removed bug Fixes for quality problems that affect the customer experience labels Jan 11, 2021
nreese
nreese reviewed Jan 11, 2021
View reviewed changes
x-pack/plugins/maps/public/plugin.ts Outdated
@@ -158,8 +158,8 @@ export class MapsPlugin
});
}

public start(core: CoreStart, plugins: MapsPluginStartDependencies): MapsStartApi {
setLicensingPluginStart(plugins.licensing);
public async start(core: CoreStart, plugins: MapsPluginStartDependencies): Promise<MapsStartApi> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought async was getting removed from start contract?

@joshdover can you add some context about plugin methods and async signatures?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is getting removed, probably at 8.0. Motivation is here: https://github.com/elastic/kibana/blob/master/rfcs/text/0007_lifecycle_unblocked.md

Tracking issue: #53268

nickpeihl
nickpeihl approved these changes Jan 11, 2021
View reviewed changes
Copy link
Member

@nickpeihl nickpeihl left a comment

Choose a reason for hiding this comment

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

lgtm if the start method can truly be async.

code review and tested in firefox

nreese
nreese approved these changes Jan 14, 2021
View reviewed changes
Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

LGTM
code review

x-pack/plugins/maps/public/licensed_features.ts
let isEnterprisePlus: boolean = false;

export const getLicenseId = () => licenseId;
export const getIsGoldPlus = () => isGoldPlus;

let initializeLicense: (value: unknown) => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: how about moving initializeLicense, licenseInitialized, and whenLicenseInitialized next to setLicensingPluginStart so all of the logic sits in a single place in the file.

nickpeihl
nickpeihl approved these changes Jan 14, 2021
View reviewed changes
Copy link
Member

@nickpeihl nickpeihl left a comment

Choose a reason for hiding this comment

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

changes lgtm.

tested with firefox

@thomasneirynck
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
maps 2.9MB 2.9MB +63.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
maps 154.4KB 156.1KB +1.7KB

History

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

@thomasneirynck thomasneirynck merged commit dc30e94 into elastic:master Jan 19, 2021
thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request Jan 19, 2021
@thomasneirynck
[Maps] Always check license at plugin startup (elastic#87873)
1e71320
This was referenced Jan 19, 2021
Merged
Merged
thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request Jan 19, 2021
@thomasneirynck
[Maps] Always check license at plugin startup (elastic#87873)
59f3353
thomasneirynck added a commit that referenced this pull request Jan 19, 2021
@thomasneirynck
[Maps] Always check license at plugin startup (#87873) (#88735)
6108d16
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jan 21, 2021
@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

3 similar comments
@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@thomasneirynck thomasneirynck added v7.11.1 and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Jan 26, 2021
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jan 27, 2021
@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

10 similar comments
@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

thomasneirynck added a commit that referenced this pull request Feb 11, 2021
@thomasneirynck @kibanamachine
[Maps] Always check license at plugin startup (#87873) (#88739)
6643748 
Co-authored-by: Kibana Machine <[email protected]>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshdover joshdover joshdover left review comments

@nickpeihl nickpeihl nickpeihl approved these changes

@nreese nreese nreese approved these changes

Assignees
No one assigned
Labels
chore [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation release_note:fix v7.11.1 v7.12.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Maps] Should make sure license check completes at startup
6 participants
@thomasneirynck @elasticmachine @kibanamachine @nreese @nickpeihl @joshdover