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

[ML] Use doc links service for transforms #86919

Merged
merged 3 commits into from
Jan 4, 2021
Merged

Conversation

lcawl
Copy link
Contributor

@lcawl lcawl commented Dec 23, 2020

Summary

Related to #86036

This PR replaces documentation links in the transform pages with calls to the documentation link service, so that the links are all maintained in one place.

@lcawl lcawl added Feature:Transforms ML transforms v7.11.0 v7.12.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes and removed v7.11.0 labels Dec 24, 2020
@lcawl lcawl marked this pull request as ready for review December 24, 2020 01:53
@lcawl lcawl requested review from a team as code owners December 24, 2020 01:53
@@ -583,6 +583,7 @@ export interface DocLinksStart {
readonly ml: Record<string, string>;
readonly transforms: Record<string, string>;
readonly visualize: Record<string, string>;
readonly apis: Record<string, string>;
Copy link
Member

Choose a reason for hiding this comment

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

Is the intent for apis to be a general grouping of docs for all ES REST APIs? Asking because I want to make sure we know what belongs here moving forward.

Transforms have their own section, which feels a bit inconsistent, so I guess I'm wondering if we should consider something like this:

docLinks.links.transforms.putTransform
docLinks.links.indices.createIndex

Or perhaps group all ES docs generally like this:

docLinks.links.es.guides.transforms
docLinks.links.es.apis.transforms.putTransform

Either way I'm not too concerned about this ATM -- we really need to rethink how this service works anyway and it isn't difficult to change -- just thought I'd ask in case anyone has thoughts on it.

Copy link
Contributor Author

@lcawl lcawl Dec 30, 2020

Choose a reason for hiding this comment

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

At this point, my idea was that the apis grouping would contain reference material for APIs from any product or solution. In the near future, we're moving away from product-focused documentation, but we'll still have (hopefully auto-generated) reference pages for Kibana, Elasticsearch, and Cloud APIs, for example. I've added more examples in #86972. If it's preferable to include the API links under feature groupings, I'm okay with that, though I'm not sure what feature we'd put things like the "create pipeline API" under. An "ingest" grouping? Happy to consider whatever additional reorganization suggestions you have.

Copy link
Member

Choose a reason for hiding this comment

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

my idea was that the apis grouping would contain reference material for APIs from any product or solution. In the near future, we're moving away from product-focused documentation, but we'll still have (hopefully auto-generated) reference pages for Kibana, Elasticsearch, and Cloud APIs, for example.

Thanks, this is helpful context -- in that case the grouping makes a bit more sense to me.

I'm not sure what feature we'd put things like the "create pipeline API" under. An "ingest" grouping? Happy to consider whatever additional reorganization suggestions you have.

Yeah I don't necessarily have an answer to this ATM either, I suppose I was mostly trying to get a picture of what you had in mind.

It seems like it would be worth revisiting this discussion and re-evaluating how docLinks is structured further down the road, once the new docs are in place.

@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

LGTM

@lcawl
Copy link
Contributor Author

lcawl commented Jan 4, 2021

@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
transform 1.0MB 1.0MB -1.0KB

Distributable file count

id before after diff
default 47266 48026 +760

Page load bundle

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

id before after diff
core 560.8KB 560.9KB +171.0B

History

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

@lcawl lcawl merged commit 28e60d4 into elastic:master Jan 4, 2021
@lcawl lcawl deleted the transform-links branch January 4, 2021 17:59
lcawl added a commit to lcawl/kibana that referenced this pull request Jan 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Transforms ML transforms :ml release_note:skip Skip the PR/issue when compiling release notes v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants