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.

Sign up for GitHub

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

Jump to bottom

[Usage Collector] Fix schema types to allow arrays #70988

Merged
merged 3 commits into from
Jul 8, 2020
Merged

[Usage Collector] Fix schema types to allow arrays #70988

merged 3 commits into from
Jul 8, 2020

Conversation

afharo
Copy link
Member

@afharo afharo commented Jul 7, 2020 •
edited
Loading

Summary

It provides array support to the schema.

A clear note: it was already supported regarding the actual checks but Types were raising errors in the MakeSchemaFrom type definition. This PR should fix those type-related issues.

Related to #70180 and #69294

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@afharo afharo mentioned this pull request Jul 7, 2020
Merged
@afharo afharo marked this pull request as ready for review July 7, 2020 18:47
@afharo afharo requested a review from a team as a code owner July 7, 2020 18:47
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-telemetry (Team:KibanaTelemetry)

@afharo afharo requested a review from skh July 7, 2020 18:53
@afharo
Copy link
Member Author

afharo commented Jul 7, 2020

Adding @skh for awareness about the fix is almost there :)

TinaHeiligers
TinaHeiligers reviewed Jul 7, 2020
View reviewed changes
src/plugins/usage_collection/server/collector/collector.test.ts Outdated
expect(collector).toBeDefined();
});

it('should be OK with arrays returned by fetch but object-schema', () => {
Copy link
Contributor

@TinaHeiligers TinaHeiligers Jul 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I don't understand what the description for this test means. From looking at the test itself, we allow arrays of objects. Translating that to the test description, I get: should be OK with arrays of objects returned by fetch.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right! My intention in the test was to use the Unit Tests as well to catch the TS errors when the object contains array properties. I'll rephrase the description of the test. Sorry for the confusion :)

TinaHeiligers
TinaHeiligers approved these changes Jul 7, 2020
View reviewed changes
Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

I'm not a typescript expert but it looks good and works for me 😄
nit: Could we clarify the one test description a bit please?
LGTM

src/plugins/usage_collection/server/collector/collector.ts Show resolved Hide resolved
@afharo
Copy link
Member Author

afharo commented Jul 8, 2020 •
edited
Loading

@TinaHeiligers I've changed the test description and also added more test use cases (collector and usage_collector files are now 💯% covered) 🙂

Please, let me know if the new structure and descriptions are easier to understand.

@afharo afharo requested a review from TinaHeiligers July 8, 2020 10:08
@skh skh mentioned this pull request Jul 8, 2020
Closed
@skh
Copy link
Contributor

skh commented Jul 8, 2020 •
edited
Loading

I'm not sure I'm doing this right, but I'm still seeing typescript errors on the schema.

As far as I can see, this seems to fix my issues:

Here's what I did:

The result is here: #71075

@afharo
Copy link
Member Author

afharo commented Jul 8, 2020

@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

TinaHeiligers
TinaHeiligers approved these changes Jul 8, 2020
View reviewed changes
Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

Thanks for these explicit test cases!
LGTM

@afharo afharo merged commit 93ac059 into elastic:master Jul 8, 2020
@afharo afharo deleted the usage-collection/support-arrays-in-schema branch July 8, 2020 16:31
@afharo afharo mentioned this pull request Jul 8, 2020
Merged
afharo added a commit that referenced this pull request Jul 8, 2020
@afharo @elasticmachine
[7.x] [Usage Collector] Fix schema types to allow arrays (#70988) (#7…
5b31747 
…1123)

* [Usage Collector] Fix schema types to allow arrays

* More and better tests

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

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@skh skh mentioned this pull request Jul 9, 2020
Merged
@lukeelmers lukeelmers added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Oct 1, 2021
@elasticmachine
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TinaHeiligers TinaHeiligers TinaHeiligers approved these changes

@skh skh Awaiting requested review from skh

Assignees
No one assigned
Labels
Feature:Telemetry 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.9.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@afharo @elasticmachine @skh @kibanamachine @TinaHeiligers @lukeelmers