-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Enterprise Search] Add ml doc links #141921
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggested a potential refactoring, but I'm approving because the links look good.
@@ -129,6 +129,7 @@ export const getDocLinks = ({ kibanaBranch }: GetDocLinkOptions): DocLinks => { | |||
crawlerGettingStarted: `${ENTERPRISE_SEARCH_DOCS}crawler-getting-started.html`, | |||
crawlerManaging: `${ENTERPRISE_SEARCH_DOCS}crawler-managing.html`, | |||
crawlerOverview: `${ENTERPRISE_SEARCH_DOCS}crawler.html`, | |||
deployTrainedModels: `${ELASTIC_WEBSITE_URL}guide/en/machine-learning/${DOC_LINK_VERSION}/ml-nlp-deploy-models.html`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we create MACHINE_LEARNING_DOCS
, which encapsulates ${ELASTIC_WEBSITE_URL}guide/en/machine-learning/${DOC_LINK_VERSION}
?
I ask because this entry stands out as different from the others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an existing link (startTrainedModelsDeployment
) that takes you to https://www.elastic.co/guide/en/machine-learning/master/ml-nlp-deploy-models.html#ml-nlp-deploy-model. Is this new one added since you want to be taken to the top of the page?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me. There is a whole bunch of other links on the same file from L402 to L429.
To make sure I test it throughly, I will create the ${MACHINE_LEARNING_DOCS}
but merge them in a different PR (following up this one) that aims for 8.6 instead of 8.5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lcawl startTrainedModelsDeployment is pointing to an anchor on the page. I wanted to link to the full page instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chriscressman Yes, it's true that we could add a new shortcut for the ML Guide, but it's not necessary in this PR IMO. Each time a new shortcut like that is created, we also have to update the doc build to be aware of it, so it would require a matching PR in the docs repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chriscressman Good idea to add the shortcut for ML! I took note of it and will add the shortcut soon and handle the docs repo PR, too, if it's okay with you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All sounds good to me! I agree we don't need the change on this PR and can handle it separately if others think that's useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the shortcut via #142091 and elastic/docs#2534. Thanks for your patience!
@elastic/kibana-docs that would be great if I can get an approval for this one. |
@elasticmachine merge upstream |
fda82f0
to
1fc0603
Compare
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elastic/kibana-docs hello, it's me again, we would need this link for the next BC so if someone can review it before that would be great. @elastic/ent-search-docs-team Also while we added this handle, we need a reapproval from you I guess @chriscressman |
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
* Add documentation links for ml inference card and modal * Fix link (cherry picked from commit b66d12a)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation and see the Github Action logs for details |
* [Enterprise Search] Add ml doc links (#141921) * Add documentation links for ml inference card and modal * Fix link (cherry picked from commit b66d12a) * Fix link type for 8.5 Co-authored-by: Efe Gürkan YALAMAN <[email protected]>
* Add documentation links for ml inference card and modal * Fix link
* Add documentation links for ml inference card and modal * Fix link
Summary
Adds missing doc links for the Inference card and modal
Checklist