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

[Infra] Make nav react to Hosts view enabled flag changing #142477

Merged

Conversation

miltonhultgren
Copy link
Contributor

@miltonhultgren miltonhultgren commented Oct 3, 2022

Summary

Closes #140996

This PR adds an app updater and consumes the hosts view enabled setting via an Observable instead so that changes to this setting are reacted to by changing the nav and deep links in the Infra app plugin.

How to test

  • Open the Infrastructure app from the global nav
  • See that Hosts is not visible in the Observability nav
  • Go to Stack Management, Advanced Settings and find the Host views enabled setting and enable it, click Save
  • Go back to Infrastructure app and observe that Hosts is now visible in the Observability nav without refreshing the page

@miltonhultgren miltonhultgren added Feature:Metrics UI Metrics UI feature release_note:skip Skip the PR/issue when compiling release notes backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Oct 3, 2022
@miltonhultgren miltonhultgren requested a review from a team as a code owner October 3, 2022 14:11
@@ -74,19 +80,27 @@ export class Plugin implements InfraClientPluginClass {
fetchData: createMetricsFetchData(core.getStartServices),
});

const startDep$AndHostViewFlag$ = combineLatest([
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea how to name this combination

Copy link
Contributor

@matschaffer matschaffer left a comment

Choose a reason for hiding this comment

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

Pretty sure this is the first rxjs review I've done, so no idea if this is idiomatic or not. But I can confirm it works after observing it with my own eyes 😉

I even turned it back off and the nav went away as expected.

I'm a little surprised it takes that much code to show/hide a link without refresh, but I trust you wouldn't leave it any more verbose than it has to be :)

@miltonhultgren
Copy link
Contributor Author

miltonhultgren commented Oct 4, 2022

@matschaffer

I'm a little surprised it takes that much code to show/hide a link without refresh, but I trust you wouldn't leave it any more verbose than it has to be :)

I think the rub is that it's two different down streams, one for the Observability chrome and one for Kibana core, the types and deconstructing code doesn't help make it any more compact either. :/

To go further, I think if the code was written to be more fully Observable driven it would, for example, accept a stream for deepLinks instead and then it would be more slim to simply push updates there (rather than having both the update and the initial value). At least that is my limited experience with RxJS, you either go all in or you end up with a lot of glue code.

@miltonhultgren
Copy link
Contributor Author

@elasticmachine merge upstream

@miltonhultgren miltonhultgren enabled auto-merge (squash) October 4, 2022 07:15
@miltonhultgren
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #32 / analytics instrumented events from the browser Loaded Dashboard full loaded dashboard emits when hitting refresh
  • [job] [logs] FTR Configs #32 / analytics instrumented events from the browser Loaded Dashboard full loaded dashboard emits when query is set
  • [job] [logs] FTR Configs #32 / analytics instrumented events from the browser Loaded Dashboard full loaded dashboard should emit the "Loaded Dashboard" event when done loading complex dashboard

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
infra 84.6KB 84.8KB +204.0B

History

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

@miltonhultgren miltonhultgren merged commit d60acf8 into elastic:main Oct 4, 2022
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 4, 2022
…42477)

* [Infra] Make nav react to Hosts view enabled flag changing (elastic#140996)

* Move comment to more relevant location

Co-authored-by: Kibana Machine <[email protected]>
(cherry picked from commit d60acf8)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.5

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 Oct 4, 2022
…142585)

* [Infra] Make nav react to Hosts view enabled flag changing (#140996)

* Move comment to more relevant location

Co-authored-by: Kibana Machine <[email protected]>
(cherry picked from commit d60acf8)

Co-authored-by: Milton Hultgren <[email protected]>
WafaaNasr pushed a commit to WafaaNasr/kibana that referenced this pull request Oct 11, 2022
…42477)

* [Infra] Make nav react to Hosts view enabled flag changing (elastic#140996)

* Move comment to more relevant location

Co-authored-by: Kibana Machine <[email protected]>
WafaaNasr pushed a commit to WafaaNasr/kibana that referenced this pull request Oct 14, 2022
…42477)

* [Infra] Make nav react to Hosts view enabled flag changing (elastic#140996)

* Move comment to more relevant location

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Feature:Metrics UI Metrics UI feature release_note:skip Skip the PR/issue when compiling release notes v8.5.0 v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Infra Monitoring UI] use observable to watch for config changes
4 participants