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

Add Timelion deprecation warning to server log #75541

Merged
merged 5 commits into from
Aug 24, 2020
Merged

Conversation

alexwizp
Copy link
Contributor

@alexwizp alexwizp commented Aug 20, 2020

Closes: #63014

Summary

This PR closes #63014 (comment)
Deprecation message was added:
image

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@alexwizp alexwizp requested a review from stratoula August 20, 2020 11:14
@timroes timroes changed the title Add Timelion deprecation warning to the migration assistant Add Timelion deprecation warning to server log Aug 20, 2020
({ total }) =>
total &&
logger.warn(
'Timelion plugin was deprecated since 7.0, the Timelion app will be removed in 8.0.'
Copy link
Contributor

@timroes timroes Aug 20, 2020

Choose a reason for hiding this comment

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

Let's be more verbose here, that they still have timelion worksheets saved and maybe directly output the link to the docuemntation.

I'd suggest the following (cc @gchaps and @KOTungseth for final message):

The timelion application was deprecated since 7.0 and will be removed in 8.0. You still have saved timelion application worksheets, which you need to migrate manually to dashboards if you want to keep them after 8.0. See https://www.elastic.co/guide/en/kibana/master/timelion.html#timelion-deprecation for more details.

It seems that the doc_links service is not available on the serverside to generate links. That would mean we need to hardcode that link here, unless Platform team has still plans to expose it also server side (cc @joshdover could you give some details on this?)

Copy link
Contributor

@stratoula stratoula Aug 20, 2020

Choose a reason for hiding this comment

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

The warning we have on the app could be used here too as was suggested by @gchaps
Deprecated since 7.0, the Timelion app will be removed in 8.0. To continue using your Timelion worksheets, migrate them to a dashboard. See https://www.elastic.co/guide/en/kibana/master/timelion.html#timelion-deprecation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stratoula @timroes it was initially like on app. Then I updated it as Tim suggested.
Does it mean that I should return it back? Actually I like 2 options. Probably we can keep it as it

Copy link
Contributor

Choose a reason for hiding this comment

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

No don't return it back! Let's wait for @gchaps and @KOTungseth for the final message

Copy link
Contributor

Choose a reason for hiding this comment

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

@timroes, @alexwizp

I agree with @stratoula that the text in the message and docs should be the same. If this text suits your needs, then I would go with it:

Deprecated since 7.0, the Timelion app will be removed in 8.0. To continue using your Timelion worksheets, migrate them to a dashboard. See https://www.elastic.co/guide/en/kibana/master/timelion.html#timelion-deprecation.

Copy link
Contributor

@gchaps gchaps left a comment

Choose a reason for hiding this comment

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

Text LGTM

@stratoula
Copy link
Contributor

Tested it with and without sheets, works as it should 🎉

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@alexwizp alexwizp self-assigned this Aug 21, 2020
@alexwizp alexwizp added v7.10.0 v8.0.0 Feature:Timelion Timelion app and visualization Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Aug 21, 2020
@alexwizp alexwizp added the release_note:skip Skip the PR/issue when compiling release notes label Aug 21, 2020
@alexwizp alexwizp marked this pull request as ready for review August 21, 2020 21:11
@alexwizp alexwizp requested a review from a team as a code owner August 21, 2020 21:11
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

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

root = kbnTestServer.createRoot();
root = kbnTestServer.createRoot({ plugins: { initialize: false } });
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to understand, why did you have to add the { plugins: { initialize: false } } option to these core integration tests? Is that just test perf optimization?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right, that is adding an ES call during start so it would cause ES connection errors. Thanks.

Copy link
Contributor

@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.

LGTM

@alexwizp alexwizp merged commit 2620882 into elastic:master Aug 24, 2020
alexwizp added a commit to alexwizp/kibana that referenced this pull request Aug 24, 2020
* Add Timelion deprecation warning to the migration assistant

Closes: elastic#63014

* fix PR comments

* update message

* fix integration tests

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
alexwizp added a commit that referenced this pull request Aug 24, 2020
* Add Timelion deprecation warning to the migration assistant

Closes: #63014

* fix PR comments

* update message

* fix integration tests

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@stratoula stratoula mentioned this pull request Sep 9, 2021
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Timelion Timelion app and visualization release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Timelion deprecation warning to the migration assistant
7 participants