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

[RAM] Category fields endpoint #138245

Merged
merged 36 commits into from
Sep 12, 2022
Merged

Conversation

jcger
Copy link
Contributor

@jcger jcger commented Aug 8, 2022

Summary

Implements #137988 server part

Checklist

  • Functional tests will be added with the frontend pr
  • unit test

How to QA

  • The new endpoint is internal/rac/alerts/browser_fields and it expects featureIds as query param, for example curl -u 'user:password' '[HOST:PORT/BASE_PATH]/internal/rac/alerts/browser_fields?featureIds=logs&featureIds=apm' should return all field capabilities in logs and apm

@jcger jcger mentioned this pull request Aug 9, 2022
9 tasks
@jcger jcger force-pushed the 137988-category-field-api branch from ab8955f to 34a2cfd Compare August 11, 2022 08:12
@jcger
Copy link
Contributor Author

jcger commented Sep 7, 2022

@elasticmachine merge upstream

@jcger jcger marked this pull request as ready for review September 7, 2022 09:05
@jcger jcger requested review from a team as code owners September 7, 2022 09:05
@jcger jcger added Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) release_note:feature Makes this part of the condensed release notes labels Sep 7, 2022
Copy link
Contributor

@XavierM XavierM left a comment

Choose a reason for hiding this comment

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

@jcger we should create some api integration test under this folder , and can we add basic unit test too?

query: buildRouteValidation(
t.exact(
t.type({
featureIds: t.union([t.string, t.array(t.string)]),
Copy link
Contributor

Choose a reason for hiding this comment

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

@jcger That's one of the reason why your API integration was not working because before you only allow for array. However, when you just have one item in a query parameter, it is automatically converted to just be a string.

expectedStatusCode: number = 200
) => {
const resp = await supertestWithoutAuth
.get(`${getSpaceUrlPrefix(SPACE1)}${TEST_URL}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

@jcger I had to add the space because your user secOnlyRead only have access to space 1. On Monday let's go over kibana privileges together. It is always confusing for everybody.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks!

Copy link
Contributor

@XavierM XavierM left a comment

Choose a reason for hiding this comment

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

LGTM

@jcger jcger enabled auto-merge (squash) September 12, 2022 06:28
@jcger jcger disabled auto-merge September 12, 2022 06:30
Copy link
Member

@maryam-saeidi maryam-saeidi left a comment

Choose a reason for hiding this comment

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

I've tested it locally, LGTM!

import { BrowserField, BrowserFields } from '../../types';

const getFieldCategory = (fieldCapability: FieldSpec) => {
const name = fieldCapability.name.split('.');
Copy link
Member

Choose a reason for hiding this comment

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

nit: Since it is an array, names would be a better naming.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if I agree. It's still the name capability but split into one array, each element of the name would a part of the name but not a name by itself. Calling it names would mean that each part is a name but I think it isn't, not sure though

Copy link
Member

Choose a reason for hiding this comment

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

I see what you mean, maybe nameParts or nameSections? In general, it helps when for arrays, we have a plural name but up to you :)

};

export const fieldDescriptorToBrowserFieldMapper = (
fieldDescriptor: FieldSpec[]
Copy link
Member

Choose a reason for hiding this comment

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

nit: We can call this fields for this variable and when we iterate over it, use field for each item.

): BrowserFields => {
return fieldDescriptor.reduce((browserFields: BrowserFields, fieldCapability: FieldSpec) => {
const category = getFieldCategory(fieldCapability);
const field = browserFieldFactory(fieldCapability, category);
Copy link
Member

Choose a reason for hiding this comment

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

This field is browserField, right? We can name it browserField

@jcger jcger enabled auto-merge (squash) September 12, 2022 12:52
@kibana-ci
Copy link
Collaborator

💚 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
ruleRegistry 180 185 +5

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
ruleRegistry 10 11 +1
Unknown metric groups

API count

id before after diff
ruleRegistry 208 213 +5

History

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

@jcger jcger merged commit f236196 into elastic:main Sep 12, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Sep 12, 2022
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 release_note:feature Makes this part of the condensed release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants