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] improve deprecation logging, of PDF creation params #44130

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented Aug 27, 2019

Summary

Closes #43628

This PR updates messaging provided by the compatibility_shim layer of PDF job creation. As this layer needs to be removed for the next major version, this is a re-visit of how it works and how its tested.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

@tsullivan tsullivan added review (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.4.0 labels Aug 27, 2019
@tsullivan tsullivan requested a review from joelgriffith August 27, 2019 18:07

return `/app/kibana#${hash}?${queryString || ''}`;
};
Copy link
Member Author

Choose a reason for hiding this comment

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

I was able to hoist these helper functions up since a logger is passed in, and logging is not done from the helper functions

browserTimezone,
objectType,
title,
relativeUrl, // not deprecating
Copy link
Member Author

@tsullivan tsullivan Aug 27, 2019

Choose a reason for hiding this comment

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

It was an in-the-moment decision to say to keep this, and not add warning logging when it is used

@tsullivan tsullivan requested a review from a team August 27, 2019 18:30
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

return `/app/kibana#${hash}?${queryString || ''}`;
};
export function compatibilityShimFactory(server, logger) {
if (!logger) throw new Error('need that logger');
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be fixable with TS, but probably not worth investing in here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Shoot, this line was supposed to be dev-only 😱

Copy link
Contributor

@joelgriffith joelgriffith left a comment

Choose a reason for hiding this comment

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

Code LGTM. Was thinking TS might be prudent here, but not sure we should keep this around anyways.

throw new Error('savedObjectId is required to determine the title from the saved object');
}
/*
* TODO: Kibana 8.0:
Copy link
Member Author

Choose a reason for hiding this comment

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

Related: #44178

}
}
} catch (err) {
logger.error(err); // 404 for the savedObjectId, etc
Copy link
Member Author

Choose a reason for hiding this comment

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

This adds the logging that was needed for:

A single deprecation warning is logged, and then the job does NOT get created. Nothing else got logged (I did not have verbose logging turned on).

from #43628

@tsullivan
Copy link
Member Author

Was thinking TS might be prudent here, but not sure we should keep this around anyways.

I agree with not wanting to keep it around. The existence makes the difficulty of moving to TypeScript VERY difficult, because the compatibility layer changes the shape of the data that comes in from the route handler. So the definition of what makes JobParamsPDF has to be updated to have a pre-compatibility and post-compatibility version!

I've been really tempted to convert that layer to Typescript a few times, because I want to change where driver$ gets instantiated in the code, and as I've tried it, that changes what gets returned out of the execute_job compatibility layer.

We can talk more about this offline. This is getting to one of the hardest parts of improving the Reporting code

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@tsullivan tsullivan merged commit 1efe813 into elastic:master Aug 28, 2019
tsullivan added a commit to tsullivan/kibana that referenced this pull request Aug 28, 2019
…tic#44130)

* [Reporting] improve deprecation logging, of PDF creation params

* more test

* more test stuff

* note

* correctness

* undo prettier

* remove dev statement
@tsullivan tsullivan deleted the reporting/log-legacy-deprecation-improvements branch August 28, 2019 05:41
tsullivan added a commit that referenced this pull request Aug 28, 2019
…) (#44217)

* [Reporting] improve deprecation logging, of PDF creation params

* more test

* more test stuff

* note

* correctness

* undo prettier

* remove dev statement
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
(Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:skip Skip the PR/issue when compiling release notes review v7.4.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Reporting] Need Better deprecation warnings and behavior for 6.x post URLs
3 participants