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

[embeddable] remove embeddable factory methods from setup and start API #204797

Merged
merged 13 commits into from
Dec 19, 2024

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Dec 18, 2024

Part of embeddable rebuild cleanup. PR removes legacy embeddable factory registration APIs.

@nreese
Copy link
Contributor Author

nreese commented Dec 18, 2024

/ci

@nreese
Copy link
Contributor Author

nreese commented Dec 18, 2024

/ci

@nreese
Copy link
Contributor Author

nreese commented Dec 18, 2024

/ci

@nreese
Copy link
Contributor Author

nreese commented Dec 18, 2024

/ci

@nreese
Copy link
Contributor Author

nreese commented Dec 18, 2024

/ci

@nreese nreese marked this pull request as ready for review December 18, 2024 23:38
@nreese nreese requested review from a team as code owners December 18, 2024 23:38
@nreese nreese added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas release_note:skip Skip the PR/issue when compiling release notes v9.0.0 project:embeddableRebuild backport:version Backport to applied version labels v8.18.0 labels Dec 18, 2024
Copy link
Contributor

@ThomThomson ThomThomson 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! Left a few questions. Code only review. It's a good sign that we didn't have to remove or modify functional tests.

@@ -15,7 +15,7 @@ import { injectBaseEmbeddableInput } from './migrate_base_input';
export const getInjectFunction = (embeddables: CommonEmbeddableStartContract) => {
return (state: EmbeddableStateWithType, references: SavedObjectReference[]) => {
const enhancements = state.enhancements || {};
const factory = embeddables.getEmbeddableFactory(state.type);
const factory = embeddables.getEmbeddableFactory?.(state.type);
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this function was marked optional in CommonEmbeddableStartContract, but that type isn't used in the client-side definition of EmbeddableStart. I understand this function was removed from the clientside version, but is there a reason it needed to be marked optional in the serverside one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both client and server use inject so I typed getEmbeddableFactory as optional since its not provided by the client.

private appList?: ReadonlyMap<string, PublicAppInfo>;
private appListSubscription?: Subscription;
private enhancementsRegistry = new EnhancementsRegistry();
Copy link
Contributor

Choose a reason for hiding this comment

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

Good call to move the registry code into its own file rather than clogging up the plugin.

};

const getAllMigrationsFn = () =>
getAllMigrations(
Array.from(this.embeddableFactories.values()),
Array.from(this.enhancements.values()),
[],
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice temporary solution here

@botelastic botelastic bot added ci:project-deploy-observability Create an Observability project Feature:Embedding Embedding content via iFrame Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team labels Dec 19, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

Copy link
Contributor

@miloszmarcinkowski miloszmarcinkowski 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 owners review)

@elastic-vault-github-plugin-prod elastic-vault-github-plugin-prod bot requested a review from a team as a code owner December 19, 2024 14:28
@nreese
Copy link
Contributor Author

nreese commented Dec 19, 2024

@elasticmachine merge upstream

@nreese nreese removed the request for review from a team December 19, 2024 16:27
@elasticmachine
Copy link
Contributor

⏳ Build in-progress

History

@nreese nreese merged commit 3b61e7b into elastic:main Dec 19, 2024
9 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/12418480061

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Dec 19, 2024
…PI (elastic#204797)

Part of embeddable rebuild cleanup. PR removes legacy embeddable factory
registration APIs.

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
(cherry picked from commit 3b61e7b)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Dec 19, 2024
…tart API (#204797) (#204993)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[embeddable] remove embeddable factory methods from setup and start
API (#204797)](#204797)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Nathan
Reese","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-12-19T18:19:48Z","message":"[embeddable]
remove embeddable factory methods from setup and start API
(#204797)\n\nPart of embeddable rebuild cleanup. PR removes legacy
embeddable factory\r\nregistration
APIs.\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine
<[email protected]>\r\nCo-authored-by:
Elastic Machine
<[email protected]>","sha":"3b61e7bea7408e586faa4be6464e963b50385896","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Embedding","Team:Presentation","release_note:skip","v9.0.0","ci:project-deploy-observability","Team:obs-ux-infra_services","project:embeddableRebuild","backport:version","v8.18.0"],"title":"[embeddable]
remove embeddable factory methods from setup and start
API","number":204797,"url":"https://github.com/elastic/kibana/pull/204797","mergeCommit":{"message":"[embeddable]
remove embeddable factory methods from setup and start API
(#204797)\n\nPart of embeddable rebuild cleanup. PR removes legacy
embeddable factory\r\nregistration
APIs.\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine
<[email protected]>\r\nCo-authored-by:
Elastic Machine
<[email protected]>","sha":"3b61e7bea7408e586faa4be6464e963b50385896"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/204797","number":204797,"mergeCommit":{"message":"[embeddable]
remove embeddable factory methods from setup and start API
(#204797)\n\nPart of embeddable rebuild cleanup. PR removes legacy
embeddable factory\r\nregistration
APIs.\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine
<[email protected]>\r\nCo-authored-by:
Elastic Machine
<[email protected]>","sha":"3b61e7bea7408e586faa4be6464e963b50385896"}},{"branch":"8.x","label":"v8.18.0","branchLabelMappingKey":"^v8.18.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Nathan Reese <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:version Backport to applied version labels ci:project-deploy-observability Create an Observability project Feature:Embedding Embedding content via iFrame project:embeddableRebuild release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants