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

Set from & to as optional args in @kbn/grouping #213212

Merged
merged 1 commit into from
Mar 5, 2025

Conversation

albertoblaz
Copy link
Contributor

@albertoblaz albertoblaz commented Mar 5, 2025

Summary

Set from and to as optional args in getGroupingQuery, a function exposed by @kbn/grouping.

It will unblock this PR:

Motivation

getGroupingQuery returns an ES aggregation for grouping documents. This function assumes data will be queried in a certain interval of time. However, Asset Inventory needs to query data from the beginning of time because the UI will not provide any time-range filter. So in order to reuse this logic, we need to set both args as optional.

Reason for wrapping both fields in an optional timeRange record is to have either both present or both undefined, not only one of them present.

Checklist

  • Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
  • Documentation was added for features that require explanation or tutorials
  • Unit or functional tests were updated or added to match the most common scenarios
  • If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
  • This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The release_note:breaking label should be applied in these situations.
  • Flaky Test Runner was used on any tests changed
  • The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines

Risks

No risk at all. It would be a breaking change otherwise, if we had to require args that were optional before.

@albertoblaz albertoblaz added release_note:skip Skip the PR/issue when compiling release notes Team:Cloud Security Cloud Security team related backport:prev-minor Backport to (9.0) the previous minor version (i.e. one version back from main) labels Mar 5, 2025
@albertoblaz albertoblaz self-assigned this Mar 5, 2025
@albertoblaz albertoblaz marked this pull request as ready for review March 5, 2025 09:47
@albertoblaz albertoblaz requested review from a team as code owners March 5, 2025 09:47
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-cloud-security-posture (Team:Cloud Security)

@elasticmachine
Copy link
Contributor

elasticmachine commented Mar 5, 2025

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Async chunks

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

id before after diff
cloudSecurityPosture 487.3KB 487.3KB +42.0B
securitySolution 8.8MB 8.8MB +30.0B
total +72.0B

History

cc @albertoblaz

@albertoblaz albertoblaz changed the title Set from & to as optional args in @kbn/grouping Set from & to as optional args in @kbn/grouping Mar 5, 2025
Copy link
Contributor

@kapral18 kapral18 left a comment

Choose a reason for hiding this comment

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

LGTM for explore

Copy link
Member

@umbopepato umbopepato left a comment

Choose a reason for hiding this comment

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

@kbn/grouping changes LGTM!

@albertoblaz albertoblaz enabled auto-merge (squash) March 5, 2025 15:12
Copy link
Contributor

@seanrathier seanrathier left a comment

Choose a reason for hiding this comment

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

Nice change using a timeRange object

@albertoblaz albertoblaz merged commit af147b5 into elastic:main Mar 5, 2025
16 checks passed
@albertoblaz albertoblaz deleted the optional-timerange branch March 5, 2025 16:22
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 9.0

https://github.com/elastic/kibana/actions/runs/13680590239

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Mar 5, 2025
## Summary

Set `from` and `to` as optional args in `getGroupingQuery`, a function
exposed by `@kbn/grouping`.

It will unblock this PR:
- elastic#212955

### Motivation

`getGroupingQuery` returns an ES aggregation for grouping documents.
This function assumes data will be queried in a certain interval of
time. However, Asset Inventory needs to query data from the beginning of
time because the UI will not provide any time-range filter. So in order
to reuse this logic, we need to set both args as optional.

Reason for wrapping both fields in an optional `timeRange` record is to
have either both present or both undefined, not only one of them
present.

### Checklist

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [x] This was checked for breaking HTTP API changes, and any breaking
changes have been approved by the breaking-change committee. The
`release_note:breaking` label should be applied in these situations.
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [x] The PR description includes the appropriate Release Notes section,
and the correct `release_note:*` label is applied per the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

### Risks

No risk at all. It would be a breaking change otherwise, if we had to
require args that were optional before.

(cherry picked from commit af147b5)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
9.0

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Mar 5, 2025
)

# Backport

This will backport the following commits from `main` to `9.0`:
- [Set from & to as optional args in @kbn/grouping
(#213212)](#213212)

<!--- Backport version: 9.6.6 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sorenlouv/backport)

<!--BACKPORT [{"author":{"name":"Alberto
Blázquez","email":"albertoblaz@users.noreply.github.com"},"sourceCommit":{"committedDate":"2025-03-05T16:21:54Z","message":"Set
from & to as optional args in @kbn/grouping (#213212)\n\n##
Summary\n\nSet `from` and `to` as optional args in `getGroupingQuery`, a
function\nexposed by `@kbn/grouping`.\n\nIt will unblock this PR:\n-
https://github.com/elastic/kibana/pull/212955\n\n### Motivation
\n\n`getGroupingQuery` returns an ES aggregation for grouping
documents.\nThis function assumes data will be queried in a certain
interval of\ntime. However, Asset Inventory needs to query data from the
beginning of\ntime because the UI will not provide any time-range
filter. So in order\nto reuse this logic, we need to set both args as
optional.\n\nReason for wrapping both fields in an optional `timeRange`
record is to\nhave either both present or both undefined, not only one
of them\npresent.\n\n### Checklist\n\n- [ ] Any text added follows
[EUI's
writing\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\nsentence case text and includes
[i18n\nsupport](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)\n-
[
]\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\nwas
added for features that require explanation or tutorials\n- [x] [Unit or
functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere
updated or added to match the most common scenarios\n- [ ] If a plugin
configuration key changed, check if it needs to be\nallowlisted in the
cloud and added to the
[docker\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\n-
[x] This was checked for breaking HTTP API changes, and any
breaking\nchanges have been approved by the breaking-change committee.
The\n`release_note:breaking` label should be applied in these
situations.\n- [ ] [Flaky
Test\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\nused on any tests changed\n- [x] The PR description includes the
appropriate Release Notes section,\nand the correct `release_note:*`
label is applied per
the\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\n\n###
Risks\n\nNo risk at all. It would be a breaking change otherwise, if we
had to\nrequire args that were optional
before.","sha":"af147b5cc6612bc9e5632c2004f868f8c2b2a7f9","branchLabelMapping":{"^v9.1.0$":"main","^v8.19.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:Cloud
Security","backport:prev-minor","v9.1.0"],"title":"Set from & to as
optional args in
@kbn/grouping","number":213212,"url":"https://github.com/elastic/kibana/pull/213212","mergeCommit":{"message":"Set
from & to as optional args in @kbn/grouping (#213212)\n\n##
Summary\n\nSet `from` and `to` as optional args in `getGroupingQuery`, a
function\nexposed by `@kbn/grouping`.\n\nIt will unblock this PR:\n-
https://github.com/elastic/kibana/pull/212955\n\n### Motivation
\n\n`getGroupingQuery` returns an ES aggregation for grouping
documents.\nThis function assumes data will be queried in a certain
interval of\ntime. However, Asset Inventory needs to query data from the
beginning of\ntime because the UI will not provide any time-range
filter. So in order\nto reuse this logic, we need to set both args as
optional.\n\nReason for wrapping both fields in an optional `timeRange`
record is to\nhave either both present or both undefined, not only one
of them\npresent.\n\n### Checklist\n\n- [ ] Any text added follows
[EUI's
writing\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\nsentence case text and includes
[i18n\nsupport](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)\n-
[
]\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\nwas
added for features that require explanation or tutorials\n- [x] [Unit or
functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere
updated or added to match the most common scenarios\n- [ ] If a plugin
configuration key changed, check if it needs to be\nallowlisted in the
cloud and added to the
[docker\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\n-
[x] This was checked for breaking HTTP API changes, and any
breaking\nchanges have been approved by the breaking-change committee.
The\n`release_note:breaking` label should be applied in these
situations.\n- [ ] [Flaky
Test\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\nused on any tests changed\n- [x] The PR description includes the
appropriate Release Notes section,\nand the correct `release_note:*`
label is applied per
the\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\n\n###
Risks\n\nNo risk at all. It would be a breaking change otherwise, if we
had to\nrequire args that were optional
before.","sha":"af147b5cc6612bc9e5632c2004f868f8c2b2a7f9"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/213212","number":213212,"mergeCommit":{"message":"Set
from & to as optional args in @kbn/grouping (#213212)\n\n##
Summary\n\nSet `from` and `to` as optional args in `getGroupingQuery`, a
function\nexposed by `@kbn/grouping`.\n\nIt will unblock this PR:\n-
https://github.com/elastic/kibana/pull/212955\n\n### Motivation
\n\n`getGroupingQuery` returns an ES aggregation for grouping
documents.\nThis function assumes data will be queried in a certain
interval of\ntime. However, Asset Inventory needs to query data from the
beginning of\ntime because the UI will not provide any time-range
filter. So in order\nto reuse this logic, we need to set both args as
optional.\n\nReason for wrapping both fields in an optional `timeRange`
record is to\nhave either both present or both undefined, not only one
of them\npresent.\n\n### Checklist\n\n- [ ] Any text added follows
[EUI's
writing\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\nsentence case text and includes
[i18n\nsupport](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)\n-
[
]\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\nwas
added for features that require explanation or tutorials\n- [x] [Unit or
functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere
updated or added to match the most common scenarios\n- [ ] If a plugin
configuration key changed, check if it needs to be\nallowlisted in the
cloud and added to the
[docker\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\n-
[x] This was checked for breaking HTTP API changes, and any
breaking\nchanges have been approved by the breaking-change committee.
The\n`release_note:breaking` label should be applied in these
situations.\n- [ ] [Flaky
Test\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\nused on any tests changed\n- [x] The PR description includes the
appropriate Release Notes section,\nand the correct `release_note:*`
label is applied per
the\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\n\n###
Risks\n\nNo risk at all. It would be a breaking change otherwise, if we
had to\nrequire args that were optional
before.","sha":"af147b5cc6612bc9e5632c2004f868f8c2b2a7f9"}}]}]
BACKPORT-->

Co-authored-by: Alberto Blázquez <albertoblaz@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (9.0) the previous minor version (i.e. one version back from main) release_note:skip Skip the PR/issue when compiling release notes Team:Cloud Security Cloud Security team related v9.0.0 v9.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants