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

remove legacy doc links file #94274

Merged
merged 11 commits into from
Mar 17, 2021

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Mar 10, 2021

Summary

Remove the legacy doc link stub file we added in #76354 as elastic/docs#1953 added lookup from core's src/core/public/doc_links/doc_links_service.ts

@pgayvallet pgayvallet added this to Pending Review in kibana-core [DEPRECATED] via automation Mar 10, 2021
@pgayvallet pgayvallet added chore 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.13.0 labels Mar 10, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@pgayvallet pgayvallet requested review from a team March 10, 2021 10:43
@pgayvallet
Copy link
Contributor Author

pgayvallet commented Mar 10, 2021

@elastic/kibana-docs can you confirm that the elasticsearch-ci/docs job will be alright with this change?

The failure ignored because this PR doesn't contain docs message on the job scares me.

@pgayvallet pgayvallet requested a review from a team as a code owner March 10, 2021 11:37
Copy link
Contributor

@lcawl lcawl left a comment

Choose a reason for hiding this comment

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

LGTM

@pgayvallet
Copy link
Contributor Author

This PR is currently blocked because removing the legacy doc file actually cause the doc build validation script to correctly uses core's docLink service (which was never the case until now), revealing a few issues:

It seems like there are three types of broken links in docs_links_service.ts:

  • Links that start with ${ELASTIC_WEBSITE_URL} but go to places other than the docs (like blog pages or marketing pages). We can’t check these links as part of the docs build, so we should ignore these. I’ll work on a change in elastic/docs.
  • Links to the enterprise-search docs. These don’t work because we don’t publish the master branch of those docs. I’m not sure what to do with these.
  • Actual broken links to things under en/kibana which should be fixed

Currently broken links are:

06:44:04 INFO:build_docs:  Kibana [master]: src/core/public/doc_links/doc_links_service.ts contains broken links to:
06:44:04 INFO:build_docs:   - ${ELASTIC_WEBSITE_URL}blog/external-collection-for-elastic-stack-monitoring-is-now-available-via-metricbeat
06:44:04 INFO:build_docs:   - ${ELASTIC_WEBSITE_URL}maps
06:44:04 INFO:build_docs:   - ${ELASTIC_WEBSITE_URL}what-is/kibana-lens
06:44:04 INFO:build_docs:   - en/app-search/master
06:44:04 INFO:build_docs:   - en/enterprise-search/master
06:44:04 INFO:build_docs:   - en/kibana/master/alert-type-index-threshold.html#index-action-configuration
06:44:04 INFO:build_docs:   - en/kibana/master/tutorial-load-dataset.html
06:44:04 INFO:build_docs:   - en/workplace-search/master

Links from category 3, which need actual fixing from the docLink definition, are:

06:44:04 INFO:build_docs:   - en/kibana/master/alert-type-index-threshold.html#index-action-configuration
06:44:04 INFO:build_docs:   - en/kibana/master/tutorial-load-dataset.html

cc @gtback

@pgayvallet
Copy link
Contributor Author

pgayvallet commented Mar 11, 2021

en/kibana/master/tutorial-load-dataset.html

This one is indexPatterns.loadingData

loadingData: `${ELASTIC_WEBSITE_URL}guide/en/kibana/${DOC_LINK_VERSION}/tutorial-load-dataset.html`,

Link is indeed a 404 on the site. It seems the link is unused in the code base too, so I will just remove it - 0bb1877

en/kibana/master/alert-type-index-threshold.html#index-action-configuration

This one is alerting.indexThreshold

indexThreshold: `${ELASTIC_WEBSITE_URL}guide/en/kibana/${DOC_LINK_VERSION}/alert-type-index-threshold.html#index-action-configuration`,

used here:

documentationUrl: (docLinks) => docLinks.links.alerting.indexThreshold,

cc @elastic/kibana-alerting-services Do you mind fixing that one, or telling me what to replace the target with?

EDIT: it seems https://www.elastic.co/guide/en/kibana/master/alert-type-index-threshold.html#index-action-configuration do exist, but the anchor (#index-action-configuration) is not found on the page. Should we just remove it?

Removed the anchor - b77eb21

@ymao1
Copy link
Contributor

ymao1 commented Mar 11, 2021

EDIT: it seems https://www.elastic.co/guide/en/kibana/master/alert-type-index-threshold.html#index-action-configuration do exist, but the anchor (#index-action-configuration) is not found on the page. Should we just remove it?

Yes, it should be fine to remove just the anchor.

@pgayvallet
Copy link
Contributor Author

@gtback I fixed the two link that were targeting Kibana docs.

Could you please check the CI doc build job to confirm that there are no more link from the Actual broken links to things under en/kibana which should be fixed category?

Do you also have any visibility on when the team will be able to fix the two other categories?

@gtback
Copy link
Member

gtback commented Mar 17, 2021

@elasticmachine merge upstream

@gtback
Copy link
Member

gtback commented Mar 17, 2021

@pgayvallet these are the remaining broken links:

08:12:36 INFO:build_docs:  Kibana [master]: src/core/public/doc_links/doc_links_service.ts contains broken links to:
08:12:36 INFO:build_docs:   - en/elasticsearch/reference/master/search-aggregations.html#_values_source
08:12:36 INFO:build_docs:   - en/kibana/master/alert-type-es-query.html
08:12:36 INFO:build_docs:   - en/kibana/master/alert-type-index-threshold.html
08:12:36 INFO:build_docs:   - en/kibana/master/pre-configured-action-types-and-connectors.html

@pgayvallet
Copy link
Contributor Author

08:12:36 INFO:build_docs:   - en/elasticsearch/reference/master/search-aggregations.html#_values_source

scriptedFields.scriptAggs. Invalid anchor, will remove it

08:12:36 INFO:build_docs:   - en/kibana/master/alert-type-es-query.html

alerting.esQuery

08:12:36 INFO:build_docs:   - en/kibana/master/alert-type-index-threshold.html

alerting.indexThreshold

08:12:36 INFO:build_docs:   - en/kibana/master/pre-configured-action-types-and-connectors.html

alerting.preconfiguredConnectors

These 3 are redirecting to real 404 though. @elastic/kibana-alerting-services Could you take a look and tell me what the correct urls would be?

@ymao1
Copy link
Contributor

ymao1 commented Mar 17, 2021

These 3 are redirecting to real 404 though. @elastic/kibana-alerting-services Could you take a look and tell me what the correct urls would be?

Those were changed as part of the terminology update PR. Sorry for not updating them in that PR!

  • en/kibana/master/alert-type-es-query.html -> en/kibana/master/rule-type-es-query.html
  • en/kibana/master/alert-type-index-threshold.html -> en/kibana/master/rule-type-index-threshold.html
  • en/kibana/master/pre-configured-action-types-and-connectors.html -> en/kibana/master/pre-configured-connectors.html

@pgayvallet pgayvallet force-pushed the kbn-xxx-remove-legacy-doc-link branch from c46074f to e8bce0f Compare March 17, 2021 13:59
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Page load bundle

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

id before after diff
core 433.5KB 433.4KB -160.0B

History

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

@pgayvallet pgayvallet merged commit 5669fac into elastic:master Mar 17, 2021
kibana-core [DEPRECATED] automation moved this from Pending Review to Done (7.13) Mar 17, 2021
pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Mar 17, 2021
* remove legacy doc links file

* remove entry from watch paths

* remove unused invalid link

* fix alerting.indexThreshold link

* fix more links

* update generated doc

* update generated doc
pgayvallet added a commit that referenced this pull request Mar 19, 2021
* remove legacy doc links file (#94274)

* remove legacy doc links file

* remove entry from watch paths

* remove unused invalid link

* fix alerting.indexThreshold link

* fix more links

* update generated doc

* update generated doc

* update generated doc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore 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.13.0
Projects
Development

Successfully merging this pull request may close these issues.

None yet

8 participants