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

[Monitoring][Alerting] Added core features to Kibana services #84486

Merged
merged 6 commits into from
Dec 4, 2020

Conversation

igoristic
Copy link
Contributor

Resolves #84483

Accommodates for changes made in #83248

@igoristic igoristic added Team:Monitoring Stack Monitoring team v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.11.0 labels Nov 30, 2020
@igoristic igoristic requested a review from a team November 30, 2020 11:22
@elasticmachine
Copy link
Contributor

Pinging @elastic/stack-monitoring (Team:Monitoring)

@chrisronline
Copy link
Contributor

@igoristic Nice work detecting this.

I think we should definitely add a test at this point. Something that will fail when other teams make changes that break our usage of alerting.

Maybe we can get away with a unit test that renders our AlertsPanel and makes sure it doesn't error out.

WDYT?

Copy link
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.

@igoristic
Copy link
Contributor Author

@chrisronline

If I load up your PR, and comment out ...

The test was only intended to pass core features to the alert form, so if the alert form requires something else (or new) it would break. But, come to think of it I don't think this test is actually possible as a unit test. Since, we'd still need to mock Legacy shims with core services, via:

jest.mock('../legacy_shims', () => ({
  Legacy: {
    shims: {
      kibanaServices: {
        ...coreMock.createStart(),
      }
    }
  },
}));

Which still makes it bias. Do you have any ideas or suggestions?

@chrisronline
Copy link
Contributor

Is it possible to use mount from enzyme and render our <AlertPanel>, then trigger the popover to show and make sure it loads properly?

Or, we can look into adding a functional test for it?

Also, feel free to make an issue and do this after this too if it will hold up the PR

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

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
monitoring 952.4KB 952.4KB +4.0B

History

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

Copy link
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! Great work!

@brianseeders
Copy link
Contributor

@igoristic I had to revert this PR because of an incompatibility with something else that was merged. See here for type check failures. Can you fix that up and re-submit?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes reverted Team:Monitoring Stack Monitoring team v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Monitoring] Fix a breaking change from alerts' action form
5 participants