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

Move reporting job types to /common #114400

Merged
merged 21 commits into from
Nov 3, 2021

Conversation

vadimkibana
Copy link
Contributor

@vadimkibana vadimkibana commented Oct 8, 2021

Summary

Closes #107085

  • Moves job param types to /common folder.
  • Re-exports job param types on browser and server.
  • Makes reporting example use JobParamsPDFV2 type.
  • Makes canvas use JobParamsPDFV2 type.

For maintainers

@vadimkibana vadimkibana marked this pull request as ready for review October 11, 2021 08:41
@vadimkibana vadimkibana requested review from a team as code owners October 11, 2021 08:41
@vadimkibana
Copy link
Contributor Author

@elasticmachine merge upstream

@vadimkibana vadimkibana added (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:skip Skip the PR/issue when compiling release notes review Team:AppServicesUx v7.16.0 v8.0.0 labels Oct 11, 2021
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
reporting 134 156 +22

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
canvas 1.0MB 1.0MB +30.0B

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
reporting 12 14 +2
Unknown metric groups

API count

id before after diff
reporting 135 159 +24

History

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

Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

We should make sure the types are not requiring the apps to provide info that Reporting can get internally.

Copy link
Contributor

@crob611 crob611 left a comment

Choose a reason for hiding this comment

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

Presentation Changes LGTM

@vadimkibana
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
reporting 136 160 +24

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
reporting 13 15 +2
Unknown metric groups

API count

id before after diff
reporting 137 163 +26

History

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

Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

LGTM

It looks like there is an unused FakeRequest interface in x-pack/plugins/reporting/common/types/export_types/csv_searchsource_immediate.ts - it stood out to me because "immediate" export uses the live request, not a reconstructed one.

I'm OK with addressing that cleanup later.

@vadimkibana vadimkibana added the auto-backport Deprecated - use backport:version if exact versions are needed label Nov 3, 2021
@vadimkibana vadimkibana merged commit 0d0e17b into elastic:main Nov 3, 2021
@kibanamachine
Copy link
Contributor

The following labels were identified as gaps in your version labels and will be added automatically:

  • v8.1.0

If any of these should not be on your pull request, please manually remove them.

kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Nov 3, 2021
* move pdf types to /common folder

* move pdf v2 types to /common folder

* move png v2 types to /common folder

* move png types to /common

* move csv_searchsource_immediate types to /common

* move csv_searchsource type sto /common

* move csv types to /common folder

* export job params types on server and client

* use JobParamsPDF in example app

* use JobParamsPDFV2 in Canvas

* dont export twice

* export JobId

* improve export syntax

* update jest snapshot

* fix imports

* add JobAppParamsPDFV2 type

* add JobAppParamsPDF type

* update test snapshot

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

💔 Backport failed

Status Branch Result
8.0
7.16 Commit could not be cherrypicked due to conflicts

Successful backport PRs will be merged automatically after passing CI.

To backport manually run:
node scripts/backport --pr 114400

kibanamachine added a commit that referenced this pull request Nov 3, 2021
* move pdf types to /common folder

* move pdf v2 types to /common folder

* move png v2 types to /common folder

* move png types to /common

* move csv_searchsource_immediate types to /common

* move csv_searchsource type sto /common

* move csv types to /common folder

* export job params types on server and client

* use JobParamsPDF in example app

* use JobParamsPDFV2 in Canvas

* dont export twice

* export JobId

* improve export syntax

* update jest snapshot

* fix imports

* add JobAppParamsPDFV2 type

* add JobAppParamsPDF type

* update test snapshot

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Vadim Kibana <82822460+vadimkibana@users.noreply.github.com>
tsullivan pushed a commit to tsullivan/kibana that referenced this pull request Nov 29, 2021
* move pdf types to /common folder

* move pdf v2 types to /common folder

* move png v2 types to /common folder

* move png types to /common

* move csv_searchsource_immediate types to /common

* move csv_searchsource type sto /common

* move csv types to /common folder

* export job params types on server and client

* use JobParamsPDF in example app

* use JobParamsPDFV2 in Canvas

* dont export twice

* export JobId

* improve export syntax

* update jest snapshot

* fix imports

* add JobAppParamsPDFV2 type

* add JobAppParamsPDF type

* update test snapshot

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	x-pack/plugins/reporting/common/types/export_types/csv.ts
#	x-pack/plugins/reporting/server/index.ts
@tsullivan
Copy link
Member

I saw that the attempted backport to 7.16 was not completed due to conflicts. So I put together this re-attempt: #119870

tsullivan added a commit that referenced this pull request Dec 1, 2021
* Move reporting job types to /common (#114400)

* move pdf types to /common folder

* move pdf v2 types to /common folder

* move png v2 types to /common folder

* move png types to /common

* move csv_searchsource_immediate types to /common

* move csv_searchsource type sto /common

* move csv types to /common folder

* export job params types on server and client

* use JobParamsPDF in example app

* use JobParamsPDFV2 in Canvas

* dont export twice

* export JobId

* improve export syntax

* update jest snapshot

* fix imports

* add JobAppParamsPDFV2 type

* add JobAppParamsPDF type

* update test snapshot

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	x-pack/plugins/reporting/common/types/export_types/csv.ts
#	x-pack/plugins/reporting/server/index.ts

* fix ts

Co-authored-by: Vadim Kibana <82822460+vadimkibana@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed (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.16.1 v8.0.0 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Reporting] job param types must be available on client
4 participants