-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Fleet] Fix fleet_server_hosts
value in fleet/settings API
#144898
[Fleet] Fix fleet_server_hosts
value in fleet/settings API
#144898
Conversation
Pinging @elastic/fleet (Team:Fleet) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
I have a broader question about this change. Are we keeping the I originally thought we wanted to deprecate it in favour of the new endpoints, since EDIT: I see from the description that other teams are using this endpoint, so my comment is intended on the long run rather than the immediate. |
It's probably worth discussing, but it would make sense to do away with the singular
We no longer have a real use case for a unified Still, this would be a breaking change, so we should work with other teams to determine an approach here. We'll also want to audit our client side code to make sure we're not relying on this settings API anywhere in Fleet's own codebase. Probably something worth discussing in the Fleet API GA issue: #123150 |
@elasticmachine merge upstream |
@@ -18,7 +18,8 @@ export default function (providerContext: FtrProviderContext) { | |||
const esClient = getService('es'); | |||
const esArchiver = getService('esArchiver'); | |||
|
|||
describe('Settings - update', async function () { | |||
// Skipped as the Fleet Server hosts settings values are no longer used as of https://github.com/elastic/kibana/issues/137785 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the right move to skip these tests - these are leftover from when we were using the /settings
API to create new Fleet Server hosts. AFAIK this is no longer supported, as any Fleet Server Hosts within the Fleet settings saved object will be ignored. @nchaulet please correct me if I'm wrong here.
We'll probably want to remove this API altogether as part of the GA work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we probably want to remove these tests, and this API (at least the fleet_server_hosts
) part should be removed as part of the GA work (we should already mark them as deprecated in the open API spec and point to new fleet server hosts endpoints)
throw err; | ||
}); | ||
const fleetServerHosts = await listFleetServerHosts(soClient); | ||
const numHostsUrls = fleetServerHosts.total; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I update this telemetry collector to use the new Fleet Server Hosts model for its calculation instead of the settings SO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch 👍
💚 Build Succeeded
Metrics [docs]Unknown metric groupsESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @kpollich |
Summary
Resolve
fleet_server_hosts
from the new Fleet Server Host SO type instead of the settings SO.Would like to add an integration test for this, but requires setting up a new "get settings" suite. Will push a follow-up commit.
Fixes elastic/fleet-server#2068
Checklist