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

[Reporting] Restore legacy compatibility shim for PDF job creation #108271

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented Aug 11, 2021

Resolves #108118

This PR reverts a breaking change Reporting had in the master branch, but not 7.x. This is a low-risk PR: when it is backported to 7.x it will largely be clean-up since logic already exists there, but uses JS not TypeScript.

7.x versions have a "compatibility shim" that gives support for POST URLs that were copied in Kibana 6.2 and below. This shim was removed from the master branch in fef8485 as a way to ease the TypeScript conversion of export types and improve maintainability. New initiatives emphasize a more careful approach to removing support for deprecated features. Before removing this support we will collect telemetry to understand the impact to users.

This PR restores the behavior from 7.x, which is to fully support a URL that was copied in Kibana 6.2.4 or below.

Testing

  1. Start a cluster with Elasticsearch and Kibana version 6.2.4
  2. Import data and create a visualization. Save the visualization and copy the POST URL from the Reporting menu
    image
  3. Confirm that the POST URL has a savedObjectId parameter, and not a relativeUrls parameter
  4. Upgrade the cluster to 7.14.0.
    • Note that the POST URL copied in 6.2.4 still works
  5. Upgrading to 8.0 can be tricky, because the existence of system indices that were created in 6.2.4 will not allow ES 8.0 to run. To ease testing, I did the following:
    1. Export all of the Kibana saved objects
    2. Start a new ES using the latest 8.0-snapshot, and Kibana running this branch of Kibana.
    3. Import the original test data and the saved objects that were exported in 7.14.
  6. Confirm that the POST URL copied in 6.2.4 still works.
  7. Confirm that the job listing shows an appropriate warning:
    image

Resources:
Docker-compose config: https://gist.github.com/tsullivan/07ce488527265e9ddbdb5f0e7f5df13e
Test data to import: https://gist.github.com/tsullivan/1f999db105499e553aee020ab21c1e9b
Example saved objects: https://gist.github.com/tsullivan/9247922158b077c449415bc429ff5e7e
Script to fire post url: https://gist.github.com/tsullivan/0b86d5e1165f6460a876b4f63012fdd0

@tsullivan tsullivan force-pushed the reporting/restore-pdf-createjob-compatibility-shim branch 9 times, most recently from 52360d9 to cc6c7c9 Compare August 13, 2021 17:08
@tsullivan tsullivan force-pushed the reporting/restore-pdf-createjob-compatibility-shim branch from cc6c7c9 to f455996 Compare August 13, 2021 20:17
export interface TaskPayloadPDF extends BasePayload {
layout: LayoutParams;
forceNow?: string;
objects: Array<{ relativeUrl: string }>;
Copy link
Member Author

Choose a reason for hiding this comment

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

Note, previously in the master branch, this TaskPayloadPDF field was called relativeUrls, which matches the interface data in the incoming BaseParamsPDF data. This was done to ease TypeScript conversion of the PDF export type: the compatibility shim was not converted into TypeScript until now.

* 2.0.
*/

import type { KibanaRequest } from 'kibana/server';
Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -28,7 +28,7 @@ const getMockJob = (base: object) => base as TaskPayloadPNG & TaskPayloadPDF;
test(`fails if no URL is passed`, async () => {
const fn = () => getFullUrls(mockConfig, getMockJob({}));
expect(fn).toThrowErrorMatchingInlineSnapshot(
`"No valid URL fields found in Job Params! Expected \`job.relativeUrl: string\` or \`job.relativeUrls: string[]\`"`
`"No valid URL fields found in Job Params! Expected \`job.relativeUrl\` or \`job.objects[{ relativeUrl }]\`"`
Copy link
Member Author

Choose a reason for hiding this comment

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

Restore the legacy shim for PDF job creation from the 7.x branch.
throw new Error('Invalid Relative URL in relativeUrls. String is expected.');
}
});

Copy link
Member Author

Choose a reason for hiding this comment

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

@tsullivan tsullivan force-pushed the reporting/restore-pdf-createjob-compatibility-shim branch from f455996 to 0007a27 Compare August 13, 2021 21:29
@tsullivan tsullivan marked this pull request as ready for review August 14, 2021 00:50
@tsullivan tsullivan added release_note:skip Skip the PR/issue when compiling release notes Team:AppServices v7.15.0 v8.0.0 labels Aug 14, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServices)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-reporting-services (Team:Reporting Services)

@tsullivan
Copy link
Member Author

@elasticmachine merge upstream

}
}

isDeprecated = true;
Copy link
Member Author

Choose a reason for hiding this comment

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

After https://github.com/elastic/kibana/pull/108614/files, we can collect telemetry for any export type that has this flag set to true in the job payload.

@tsullivan
Copy link
Member Author

@elasticmachine merge upstream

Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

Code changes look good to me! I tested and the compatibility shim seems to work as expected.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@tsullivan tsullivan merged commit 468daeb into elastic:master Aug 17, 2021
@tsullivan tsullivan deleted the reporting/restore-pdf-createjob-compatibility-shim branch August 17, 2021 20:01
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 108271 or prevent reminders by adding the backport:skip label.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Aug 19, 2021
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 108271 or prevent reminders by adding the backport:skip label.

1 similar comment
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 108271 or prevent reminders by adding the backport:skip label.

@tsullivan tsullivan added the backport:skip This commit does not require backporting label Aug 24, 2021
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Aug 24, 2021
@tsullivan
Copy link
Member Author

This PR was effectively backported in #108979

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:skip Skip the PR/issue when compiling release notes v7.15.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Reporting] Restore the PDF layer for 6.2 URL compatibility
5 participants