Skip to content

Use newfeed.service config for all newsfeeds#90252

Merged
gtback merged 3 commits intoelastic:masterfrom
gtback:custom-newsfeed-domains
Feb 4, 2021
Merged

Use newfeed.service config for all newsfeeds#90252
gtback merged 3 commits intoelastic:masterfrom
gtback:custom-newsfeed-domains

Conversation

@gtback
Copy link
Copy Markdown
Member

@gtback gtback commented Feb 3, 2021

Summary

Prior to this change, the main kibana newsfeed would respect the
newsfeed.service.urlRoot config setting, but other feeds like
kibana-analytics would not. This made it difficult to test new feed
items using a local server.

Checklist

Delete any items that are not applicable to this PR.

  • [?] Documentation was added for features that require explanation or tutorials
  • [?] Unit or functional tests were updated or added to match the most common scenarios
  • [NA, I think] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list

I'm not sure if there's a place this should get documented, and I couldn't find any tests that cover this configuration; but I'm happy to add either if I need to. The config key itself isn't changing, it's just being used in more places.

I noticed there's a separate setting securitySolution:newsFeedUrl which is related, too.

For maintainers

CC: @dsmith001 @alexfrancoeur

@gtback
Use newfeed.service config for all newsfeeds
773ec4e 
Prior to this change, the main `kibana` newsfeed would respect the
`newsfeed.service.urlRoot` config setting, but other feeds like
`kibana-analytics` would not. This made it difficult to test new feed
items using a local server.
@gtback gtback requested a review from a team as a code owner February 3, 2021 23:34
afharo
afharo approved these changes Feb 4, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@afharo afharo left a comment

Choose a reason for hiding this comment

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

Hey @gtback thank you for doing this change. I think they make perfect sense!

Re the tests, during functional tests, we point to a local Kibana:

kibana/test/common/config.js

Line 58 in f979580

`--newsfeed.service.urlRoot=${servers.kibana.protocol}://${servers.kibana.hostname}:${servers.kibana.port}`,

You might want to extend the test plugin to serve kibana-analytics to help with testing.

@Bamieh Bamieh added release_note:skip Skip the PR/issue when compiling release notes v7.12.0 v8.0.0 labels Feb 4, 2021
@Bamieh
Copy link
Copy Markdown
Contributor

Bamieh commented Feb 4, 2021

@elasticmachine merge upstream

@gtback
Copy link
Copy Markdown
Member Author

gtback commented Feb 4, 2021

Thanks @afharo!

You might want to extend the test plugin to serve kibana-analytics to help with testing.

If I understand this correctly, Kibana uses an internal test endpoint (/api/_newsfeed-FTS-external-service-simulators/) for running newsfeed service during tests. If anything, I'd consider refactoring the line below to parameterize the feed name (kibana). But I think this would be a more extensive refactoring, and for my use case I'm running a separate server locally with the newsfeed content.

`--newsfeed.service.pathTemplate=/api/_newsfeed-FTS-external-service-simulators/kibana/v{VERSION}.json`

If it's fine with you, I think I'll merge this once tests pass with the change you suggested above.

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Feb 4, 2021

💚 Build Succeeded

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
newsfeed 20.6KB 20.7KB +23.0B

History

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

@gtback gtback merged commit 54b1fb6 into elastic:master Feb 4, 2021
@gtback gtback deleted the custom-newsfeed-domains branch February 4, 2021 18:27
@gtback
Copy link
Copy Markdown
Member Author

gtback commented Feb 4, 2021

@afharo is there anything I need to do to backport this, or does it happen automatically?

gmmorris added a commit to gmmorris/kibana that referenced this pull request Feb 4, 2021
@gmmorris
Merge branch 'master' into task-manager/support-limited-concurrency
5190abf 
* master: (244 commits)
  [maps] Top hits per entity--change to title to use recent, minor edits (elastic#89254)
  [DOCS] Update installation details (elastic#90354)
  RFC for automatically generated typescript API documentation for every plugins public services, types, and functionality (elastic#86704)
  Elastic Maps Server config is `host` not `hostname` (elastic#90234)
  Use doc link services in index pattern management (elastic#89937)
  [Fleet] Managed Agent Policy (elastic#88688)
  [Workplace Search] Fix Source Settings bug  (elastic#90242)
  [Enterprise Search] Refactor MockRouter test helper to not store payload (elastic#90206)
  Use doc link service in more Stack Monitoring pages (elastic#89050)
  [App Search] Relevance Tuning logic - actions and selectors only, no listeners (elastic#89313)
  Remove UI filters from UI (elastic#89793)
  Use newfeed.service config for all newsfeeds (elastic#90252)
  skip flaky suite (elastic#85086)
  Add readme to geo containment alert covering test alert setup (elastic#89625)
  [APM] Enabling yesterday option when 24 hours is selected (elastic#90017)
  Test user for maps tests under import geoJSON tests (elastic#86015)
  [Lens] Hide column in table (elastic#88680)
  [Security Solution][Detections] Reduce detection engine reliance on _source (elastic#89371)
  [Discover] Minor cleanup (elastic#90260)
  [Search Session][Management] Rename "cancel" button and delete "Reload" button (elastic#90015)
  ...
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 8, 2021
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Feb 8, 2021

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 90252 or prevent reminders by adding the backport:skip label.

@gtback gtback mentioned this pull request Feb 8, 2021
Merged
gtback added a commit to gtback/kibana that referenced this pull request Feb 8, 2021
@gtback @afharo @kibanamachine
Use newfeed.service config for all newsfeeds (elastic#90252)
83c66d7 
Co-authored-by: Alejandro Fernández Haro <afharo@gmail.com>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 9, 2021
gtback added a commit that referenced this pull request Feb 9, 2021
@gtback @afharo @kibanamachine
[7.x] Use newfeed.service config for all newsfeeds (#90252) (#90681)
83ea30d 
Co-authored-by: Alejandro Fernández Haro <afharo@gmail.com>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Alejandro Fernández Haro <afharo@gmail.com>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@afharo afharo afharo approved these changes

Assignees

No one assigned

Labels

release_note:skip Skip the PR/issue when compiling release notes v7.12.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@gtback @Bamieh @kibanamachine @afharo