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

Migrate data folder creation from legacy to KP #75527

Merged
merged 15 commits into from
Aug 26, 2020

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Aug 20, 2020

Summary

Fix #73002

  • rename the uuid service to environment
  • move the data folder creation from the legacy kibana plugin to the renamed service

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 Feature:Legacy Removal Issues related to removing legacy Kibana labels Aug 20, 2020
@pgayvallet pgayvallet marked this pull request as ready for review August 20, 2020 12:38
@pgayvallet pgayvallet requested review from a team as code owners August 20, 2020 12:38
@pgayvallet pgayvallet requested a review from a team August 20, 2020 12:38
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

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

Alerting code LGTM

Copy link
Contributor

@igoristic igoristic left a comment

Choose a reason for hiding this comment

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

Stack Monitoring side looks good 👍

@pgayvallet
Copy link
Contributor Author

@elastic/kibana-telemetry the telemetry schema check fails. probably because of the reintegration of the changes from #75536, as this PR also remove things from the last remaining legacy plugin.

Seems like

{
"output": "src/plugins/telemetry/schema/legacy_oss_plugins.json",
"root": "src/legacy/core_plugins/",
"exclude": [
"src/legacy/core_plugins/testbed",
"src/legacy/core_plugins/elasticsearch",
"src/legacy/core_plugins/tests_bundle"
]
},

is no longer extracting anything:

ERROR Error: No paths covered from /dev/shm/workspace/parallel/13/kibana/src/legacy/core_plugins/ by the .telemetryrc.json

should I just remove the legacy_oss_plugins.json entry from .telemetryrc.json, or is there anything else/more to do here?

@afharo
Copy link
Member

afharo commented Aug 21, 2020

should I just remove the legacy_oss_plugins.json entry from .telemetryrc.json, or is there anything else/more to do here?

@pgayvallet I think we are safe to remove that entry completely 👍

@@ -752,6 +752,11 @@ export interface EnvironmentMode {
prod: boolean;
}

// @public
export interface EnvironmentServiceSetup {
getInstanceUuid(): string;
Copy link
Contributor

Choose a reason for hiding this comment

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

as we are here: why it's exposed as a function? the value is static

@@ -42,7 +42,7 @@ export class TaskManagerPlugin
.toPromise();

setupSavedObjects(core.savedObjects, this.config);
this.taskManagerId = core.uuid.getInstanceUuid();
this.taskManagerId = core.environment.getInstanceUuid();
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO it's confusing that environment service doesn't create Env public interface


What if we make this service internal and pass uuid as a static value to the plugin constructor
const instance = initializer(this.initializerContext);
?

I understand that it's not required for the issue you are working on, so we can rollback all the changes to the uuid service.

Copy link
Contributor Author

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

@restrry Moved instanceUuid to PluginInitializerContext. PTAL

Comment on lines +40 to +42
export interface InstanceInfo {
uuid: string;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a structure as the pid will also be added here once we migrate it from the legacy server

Comment on lines -93 to +99
public async discover() {
public async discover({ environment }: PluginsServiceDiscoverDeps) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, coreContext is created synchronously inside the Server constructor. Injecting the uuid inside it was way too heavy changes, as it would require to wait for the uuid service to setup (which itself requires the core context...). I'm now setuping the environment service before the call to plugins.discover and pass the setup contract to it to allow to access the uuid (and later, the pid)

Comment on lines 278 to +281
env: {
mode: EnvironmentMode;
packageInfo: Readonly<PackageInfo>;
instanceUuid: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

instanceUuid is now accessible via pluginInitializerContext.env.instanceUuid, and the Environment service is no longer public.

@mshustov
Copy link
Contributor

@pgayvallet LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

oss distributable file count

id value diff baseline
total 26356 +1 26355

distributable file count

id value diff baseline
total 53198 +1 53197

History

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

@pgayvallet pgayvallet merged commit eee1392 into elastic:master Aug 26, 2020
pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Aug 26, 2020
* rename uuid service to environment service

* adapt resolve_uuid to directly use the configurations

* move data folder creation to core

* update generated doc

* fix types

* fix monitoring tests

* move instanceUuid to plugin initializer context

* update generated doc
pgayvallet added a commit that referenced this pull request Aug 26, 2020
* rename uuid service to environment service

* adapt resolve_uuid to directly use the configurations

* move data folder creation to core

* update generated doc

* fix types

* fix monitoring tests

* move instanceUuid to plugin initializer context

* update generated doc
parkiino pushed a commit to parkiino/kibana that referenced this pull request Sep 1, 2020
* rename uuid service to environment service

* adapt resolve_uuid to directly use the configurations

* move data folder creation to core

* update generated doc

* fix types

* fix monitoring tests

* move instanceUuid to plugin initializer context

* update generated doc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Legacy Removal Issues related to removing legacy Kibana 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.

Migrate data folder creation to core
7 participants