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

[Alerting][Actions] Prepare for making saved objects shareable in 8.0 #107221

Closed
wants to merge 6 commits into from

Conversation

chrisronline
Copy link
Contributor

@chrisronline chrisronline commented Jul 29, 2021

Resolves #107083
Resolves #107084

This PR makes changes to ensure #107070 and #107069 can smoothly merge into master and backport into 7.x.

We are able to do this because the platform exposes the current version of Kibana so we use that to determine if we use the 8.0+ namespaceType or the 7.x namespaceType.

@chrisronline
Copy link
Contributor Author

@elasticmachine merge upstream

@chrisronline chrisronline changed the title See how CI responds to this [Alerting][Actions] Prepare for making saved objects shareable in 8.0 Aug 2, 2021
@chrisronline chrisronline marked this pull request as ready for review August 2, 2021 19:47
@chrisronline chrisronline requested a review from a team as a code owner August 2, 2021 19:47
@chrisronline chrisronline added Feature:Actions Feature:Actions/Framework Issues related to the Actions Framework Feature:Alerting Feature:Alerting/RulesFramework Issues related to the Alerting Rules Framework Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) labels Aug 2, 2021
@elasticmachine
Copy link
Contributor

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

@chrisronline chrisronline added release_note:skip Skip the PR/issue when compiling release notes v7.15.0 v8.0.0 labels Aug 2, 2021
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@ymao1
Copy link
Contributor

ymao1 commented Aug 3, 2021

Do you think #107069 and #107070 should be resolved and merged first? That way we can make sure whatever changes to resolve still work in 7.x before these changes get into 8.0? (Since all the changes would be in master?)

@chrisronline
Copy link
Contributor Author

chrisronline commented Aug 3, 2021

@ymao1

I'm not sure.

My original thinking is that we need these changes in order to even test #107069 and #107070 (otherwise, the .resolve() will return the same response all the time) so it made sense to do this work first. This PR started as a test to make sure CI would pass with only these changes and since it worked, it made sense to move forward with this work.

That way we can make sure whatever changes to resolve still work in 7.x before these changes get into 8.0? (Since all the changes would be in master?)

I'm not sure I understand this concern. Are you imagining the work we do for 7.x (#107069 and #107070) might require us to change the code in this PR?

@ymao1
Copy link
Contributor

ymao1 commented Aug 3, 2021

I'm not sure I understand this concern. Are you imagining the work we do for 7.x (ttps://github.com//issues/107069 and #107070) might require us to change the code in this PR?

@chrisronline As I understand it, the changes to switch .resolve() will go into 7.15/7.16 and should provide backwards compatible behavior by always returning outcome: exactMatch with unmigrated alerts and actions and then when we get to 8.0, these changes will be incorporated. If we merge this PR now, master, which always represents 8.0, will include the migration, so it would be harder to test locally the resolve changes which should work without the migration. Does that make sense?

@chrisronline
Copy link
Contributor Author

@ymao1

Thanks for clarifying. I think I understand now. You're worried we won't have any idea if the .resolve() changes work properly without any migration. I think that once this PR goes into master and backports into 7.x, we'll have a slight divergence in behavior on the 8.0 and 7.x branches (assuming the code in this PR goes in as-is) and we'll need to build out tests to handle that properly. I'm assuming the tests will need to self-detect if the branch is 7.x or 8.0 (which is what this PR is doing anyway) and handle the response appropriately.

If we did this work last (and the .resolve() changes first), I think we'd be in the same boat as the implementor would write tests out on master for the constant outcome : exactMatch response but unable to write any tests for any different response (as the code would just never return anything else). Then, this PR would need to change and update the same tests to handle the other outcome response type.

Maybe it's more awkward this way and I'm happy to change it - I'm not sure if one makes more sense than the other.

@chrisronline
Copy link
Contributor Author

After syncing with @ymao1 offline, I think we're going to hold off on this work until #107069 and #107070 are done. This will also effect the amount of work necessary for this PR so I'm going to close it for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Actions/Framework Issues related to the Actions Framework Feature:Actions Feature:Alerting/RulesFramework Issues related to the Alerting Rules Framework Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.15.0 v8.0.0
Projects
None yet
4 participants