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.

Sign up for GitHub

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

Jump to bottom

Use newfeed.service config for all newsfeeds #90252

Merged
merged 3 commits into from
Feb 4, 2021
Merged

Use newfeed.service config for all newsfeeds #90252

merged 3 commits into from
Feb 4, 2021

Conversation

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

src/plugins/newsfeed/public/plugin.tsx Outdated Show resolved Hide resolved
@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
Member

Bamieh commented Feb 4, 2021

@elasticmachine merge upstream

@gtback
Copy link
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
Contributor

💚 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
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
Contributor

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