Skip to content

[Actions] [8.0] Prepare for making action saved objects sharecapable.#109756

Merged
YulNaumenko merged 11 commits into
elastic:prepare-alerting-so-to-sharecapablefrom
YulNaumenko:actions-prepare-to-sharecapable
Aug 27, 2021
Merged

[Actions] [8.0] Prepare for making action saved objects sharecapable.#109756
YulNaumenko merged 11 commits into
elastic:prepare-alerting-so-to-sharecapablefrom
YulNaumenko:actions-prepare-to-sharecapable

Conversation

@YulNaumenko
Copy link
Copy Markdown
Contributor

@YulNaumenko YulNaumenko commented Aug 24, 2021 •
edited
Loading

Summary

Resolves #107083

@YulNaumenko YulNaumenko self-assigned this Aug 24, 2021
@YulNaumenko YulNaumenko added Feature:Alerting/RuleActions Issues related to the Actions attached to Rules on the Alerting Framework Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Aug 24, 2021
@YulNaumenko YulNaumenko changed the base branch from master to prepare-alerting-so-to-sharecapable August 24, 2021 03:58
@YulNaumenko YulNaumenko changed the base branch from prepare-alerting-so-to-sharecapable to master August 24, 2021 04:00
@YulNaumenko YulNaumenko marked this pull request as ready for review August 24, 2021 04:01
@YulNaumenko YulNaumenko requested a review from a team as a code owner August 24, 2021 04:01
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Aug 24, 2021

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

ymao1
ymao1 reviewed Aug 24, 2021
View reviewed changes
Copy link
Copy Markdown
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.

Some of the work is covered in this PR: #108964 and can be removed.

Comment thread x-pack/plugins/actions/server/saved_objects/migrations.ts Outdated
Comment thread x-pack/plugins/actions/server/saved_objects/migrations.ts Outdated
@YulNaumenko YulNaumenko changed the base branch from master to prepare-alerting-so-to-sharecapable August 25, 2021 23:22
@YulNaumenko YulNaumenko requested review from a team August 25, 2021 23:22
@YulNaumenko YulNaumenko requested a review from a team as a code owner August 25, 2021 23:22
@YulNaumenko YulNaumenko requested a review from a team August 25, 2021 23:22
@YulNaumenko YulNaumenko requested review from a team as code owners August 25, 2021 23:22
@YulNaumenko YulNaumenko requested a review from a team August 25, 2021 23:22
@YulNaumenko YulNaumenko requested review from a team as code owners August 25, 2021 23:22
@YulNaumenko YulNaumenko requested a review from a team August 25, 2021 23:22
@YulNaumenko YulNaumenko requested review from ymao1 and removed request for a team, academo and parkiino August 25, 2021 23:22
@YulNaumenko YulNaumenko force-pushed the actions-prepare-to-sharecapable branch from 92f9a77 to afe7ad9 Compare August 26, 2021 03:30
@YulNaumenko YulNaumenko changed the base branch from master to prepare-alerting-so-to-sharecapable August 26, 2021 03:31
ymao1
ymao1 requested changes Aug 26, 2021
View reviewed changes
Comment thread x-pack/plugins/actions/server/saved_objects/action_task_params_migrations.ts Outdated
jportner
jportner reviewed Aug 26, 2021
View reviewed changes
Comment thread x-pack/plugins/actions/server/saved_objects/index.ts
namespaceType: 'multiple-isolated',
convertToMultiNamespaceTypeVersion: '8.0.0',
mappings: mappings.action as SavedObjectsTypeMappingDefinition,
migrations: getActionsMigrations(encryptedSavedObjects),
Copy link
Copy Markdown
Contributor

@jportner jportner Aug 26, 2021

Choose a reason for hiding this comment

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

I think you need to add a no-op migration for the ACTION_SAVED_OBJECT_TYPE type too, right?

Copy link
Copy Markdown
Contributor Author

@YulNaumenko YulNaumenko Aug 26, 2021

Choose a reason for hiding this comment

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

Ah.., I forgot about this. Thank you for pointing!

jportner
jportner approved these changes Aug 26, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@jportner jportner left a comment

Choose a reason for hiding this comment

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

I added comments for a couple of nits, overall LGTM!

I checked and it appears there are already end-to-end integration tests for migrating actions and testing to ensure secrets are decrypted properly (x-pack/test/alerting_api_integration/spaces_only/tests/actions/migrations.ts), that's why CI was failing before you added the no-op migration for actions 😄
So, assuming CI passes now, those migration tests are 👍

IMO, you don't need to add a specific 8.0.0 migration integration test, but you could optionally add a comment in that file mentioning that the conversion is getting tested implicitly by virtue of the secret getting decrypted properly.

There are also end-to-end integration test for migrating action_task_params (x-pack/test/alerting_api_integration/spaces_only/tests/action_task_params/migrations.ts), but the data in the fixtures doesn't actually include any encrypted attributes. That's an area for possible improvement in the future, but is outside the scope of this PR, IMO.

Comment thread x-pack/plugins/actions/server/saved_objects/action_task_params_migrations.test.ts Outdated
Comment thread x-pack/plugins/actions/server/saved_objects/actions_migrations.test.ts Outdated
chrisronline
chrisronline approved these changes Aug 26, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@chrisronline chrisronline left a comment

Choose a reason for hiding this comment

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

LGTM!

ymao1
ymao1 approved these changes Aug 26, 2021
View reviewed changes
Copy link
Copy Markdown
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 when failing unit test is resolved

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Aug 26, 2021

💚 Build Succeeded

History

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

cc @YulNaumenko

@YulNaumenko YulNaumenko mentioned this pull request Aug 27, 2021
Merged
1 task
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

@mikecote mikecote Awaiting requested review from mikecote

+2 more reviewers

@chrisronline chrisronline chrisronline approved these changes

@jportner jportner jportner approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@YulNaumenko YulNaumenko

Labels

Feature:Alerting/RuleActions Issues related to the Actions attached to Rules on the Alerting Framework release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Actions] [8.0] Prepare for making action saved objects sharecapable

7 participants

@YulNaumenko @elasticmachine @kibanamachine @chrisronline @mikecote @jportner @ymao1