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

[Maps] Use SO-references for geo-containment alerts #114559

Merged
merged 24 commits into from
Oct 15, 2021
Merged

[Maps] Use SO-references for geo-containment alerts #114559

merged 24 commits into from
Oct 15, 2021

Conversation

thomasneirynck
Copy link
Contributor

Closes #107066

@thomasneirynck thomasneirynck added [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v7.16.0 v8.0.0 chore release_note:skip Skip the PR/issue when compiling release notes labels Oct 11, 2021
@thomasneirynck thomasneirynck mentioned this pull request Oct 11, 2021
Closed
@thomasneirynck thomasneirynck changed the title [Maps] Migrate geo-containment alerts [Maps] Use SO-references for geo-containment alerts Oct 12, 2021
@thomasneirynck thomasneirynck marked this pull request as ready for review October 12, 2021 20:48
@thomasneirynck thomasneirynck requested review from a team as code owners October 12, 2021 20:48
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis (Team:Geo)

@thomasneirynck thomasneirynck requested a review from ymao1 October 12, 2021 20:48
nreese
nreese approved these changes Oct 13, 2021
View reviewed changes
Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

LGTM, only nit comments about naming
code review

x-pack/plugins/alerting/server/saved_objects/geo_containment/migrations.ts Outdated Show resolved Hide resolved
x-pack/plugins/alerting/server/saved_objects/geo_containment/migrations.ts Outdated Show resolved Hide resolved
@thomasneirynck thomasneirynck requested a review from ymao1 October 13, 2021 21:29
ymao1
ymao1 reviewed Oct 14, 2021
View reviewed changes
Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

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

I created a geo rule in 7.15 and then migrated using this branch. I get this error:

Error injecting reference into rule params for rule id def0ee00-2c9a-11ec-9ddf-634f16c3e5be - Index "null" not found in references array

ymao1
ymao1 reviewed Oct 14, 2021
View reviewed changes
x-pack/plugins/alerting/server/saved_objects/geo_containment/migrations.ts Outdated
const { indexId, boundaryIndexId, ...otherParams } = params;
const references = [
{
name: `tracked_index_${indexId}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

The rules client prefixes rule type specific references with param: (to avoid possible collisions with other reference names), so this should be:

Suggested change
name: `tracked_index_${indexId}`,
name: `param:tracked_index_${indexId}`,

x-pack/plugins/alerting/server/saved_objects/geo_containment/migrations.ts Outdated
id: indexId as string,
},
{
name: `boundary_index_${boundaryIndexId}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
name: `boundary_index_${boundaryIndexId}`,
name: `param:boundary_index_${boundaryIndexId}`,

x-pack/plugins/alerting/server/saved_objects/geo_containment/migrations.ts Show resolved Hide resolved
ymao1
ymao1 approved these changes Oct 14, 2021
View reviewed changes
Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

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

LGTM! Verified that:

  • creating a geo rule in 7.15 (in both default and custom space) and migrating with this branch works and the rules are loaded correctly, runs after migration, and the index patterns are populated correctly when editing the rule
  • creating a geo rule in this branch works and the index pattern ids are stored in the references array.

x-pack/plugins/stack_alerts/server/alert_types/geo_containment/tests/alert_type.test.ts
@@ -43,4 +43,54 @@ describe('alertType', () => {

expect(alertType.validate?.params?.validate(params)).toBeTruthy();
});

test('injectEntityAndBoundaryIds', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe a test for extract here?

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@thomasneirynck thomasneirynck added the auto-backport Deprecated - use backport:version if exact versions are needed label Oct 15, 2021
@thomasneirynck thomasneirynck merged commit 712fac6 into elastic:master Oct 15, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 15, 2021
@thomasneirynck @kibanamachine
[Maps] Use SO-references for geo-containment alerts (elastic#114559)
c41007e
@kibanamachine kibanamachine mentioned this pull request Oct 15, 2021
Merged
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
✅ 7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Oct 15, 2021
@kibanamachine @thomasneirynck
[Maps] Use SO-references for geo-containment alerts (#114559) (#115239)
93f3ae0 
Co-authored-by: Thomas Neirynck <[email protected]>
@ymao1 ymao1 mentioned this pull request May 17, 2022
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ymao1 ymao1 ymao1 approved these changes

@nreese nreese nreese approved these changes

Assignees
No one assigned
Labels
auto-backport Deprecated - use backport:version if exact versions are needed chore [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation release_note:skip Skip the PR/issue when compiling release notes v7.16.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Maps][Alerting] Migrate to using Saved Object References array to reference index patterns from Rules
5 participants
@thomasneirynck @elasticmachine @kibanamachine @nreese @ymao1