-
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
[Uptime] Add Settings Page #53550
[Uptime] Add Settings Page #53550
Conversation
Pinging @elastic/uptime (Team:uptime) |
*/ | ||
export const getSourceSettings = async (basePath?: string, setter?: (data: unknown) => void) => { | ||
try { | ||
const { data } = await axios.get(getApiPath('/api/uptime/source_settings', basePath)); |
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.
Is there a reason not to use http.fetch instead? Our Security model heavily depends on the fact that every plugin uses core provided API
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.
Thanks for the pointer, will make the switch, this file was copied from some much older code pre NP
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 we need to do this across the whole app. We're tracking that here. I'll leave this PR as-is since we have to make a change everywhere ultimately. #56894
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
export { getSourceSettings } from './get_source_settings'; |
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.
Would you mind updating https://github.com/elastic/kibana/blob/master/.github/CODEOWNERS with uptime app ?
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.
can do
x-pack/legacy/plugins/uptime/server/rest_api/source_settings/get_source_settings.ts
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/uptime/server/lib/adapters/saved_objects/kibana_saved_objects_adapter.ts
Outdated
Show resolved
Hide resolved
💔 Build FailedHistory
To update your PR or re-run it, just comment with: |
@elasticmachine merge upstream |
): Promise<MonitorSummaryResult> { | ||
const dynamicSettings = await savedObjectsAdapter.getUptimeDynamicSettings( | ||
savedObjectsClient, | ||
undefined |
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.
why you have to pass undefined?
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 see the same case in other places as well.
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 point! The type required a second argument for no reason.
saveSucceeded: false, | ||
saveError: action.payload, |
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.
seems like typing isn't working correctly here, otherwise it should have thrown error on saveSucceeded, something we can improve for redux reducer. but unrelated to this PR.
const couldNotSaveSettingsText = i18n.translate('xpack.uptime.settings.error.couldNotSave', { | ||
defaultMessage: 'Could not save settings!', | ||
}); | ||
yield takeLatest(String(setDynamicSettings), function*(action: Action<DynamicSettings>) { | ||
try { | ||
if (!action.payload) { | ||
const err = new Error('Cannot fetch effect without a payload'); | ||
yield put(setDynamicSettingsFail(err)); | ||
|
||
kibanaService.core.notifications.toasts.addError(err, { | ||
title: couldNotSaveSettingsText, | ||
}); | ||
return; | ||
} | ||
const basePath = yield select(getBasePath); | ||
yield call(setDynamicSettingsAPI, { settings: action.payload, basePath }); | ||
yield put(setDynamicSettingsSuccess(action.payload)); | ||
kibanaService.core.notifications.toasts.addSuccess('Settings saved!'); | ||
} catch (err) { | ||
kibanaService.core.notifications.toasts.addError(err, { | ||
title: couldNotSaveSettingsText, | ||
}); | ||
yield put(setDynamicSettingsFail(err)); | ||
} |
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.
nice usage of toasts here.
return await apiService.post(apiPath, JSON.stringify(settings), DynamicSettingsSaveType); | ||
}; |
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 wonder, if you have to do JSON.stringify? doesn't kibana http post takes care of it?
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.
Nope! Removed, good fix :)
useEffect(() => { | ||
if (setBreadcrumbs) { | ||
setBreadcrumbs([makeBaseBreadcrumb(params)].concat(extraCrumbs)); | ||
} | ||
}); |
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.
you can optimize it by passing effect depencies array, that way it will be only called when extracrumbs really changes, right now this will be called each time component renders.
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.
Done!
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.
Code Looks good, just left some minor optional comments. WFG !!
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
Adds a settings page to the Uptime UI. The settings page values are per-space. The only current setting is heartbeatIndices. To test this against alternate indices try changing setup.ilm.rollover_alias in heartbeat.yml to something like alt-prefix. See the ilm docs for more details. This should be tested with read-only and write only roles. To test this in kibana try creating two users with two different roles in kibana. One roll should have read access to the Uptime space in kibana. The other should have all access. Both should have read permissions for the heartbeat-* index pattern. This patch also splits API perms from just heartbeat to uptime-read and uptime-write. This patch also refactors some of the header component functionality, using hooks for breadcrumbs, and making the top links optional. Fixes elastic/uptime#43
* [Uptime] Add Settings Page (#53550) Adds a settings page to the Uptime UI. The settings page values are per-space. The only current setting is heartbeatIndices. To test this against alternate indices try changing setup.ilm.rollover_alias in heartbeat.yml to something like alt-prefix. See the ilm docs for more details. This should be tested with read-only and write only roles. To test this in kibana try creating two users with two different roles in kibana. One roll should have read access to the Uptime space in kibana. The other should have all access. Both should have read permissions for the heartbeat-* index pattern. This patch also splits API perms from just heartbeat to uptime-read and uptime-write. This patch also refactors some of the header component functionality, using hooks for breadcrumbs, and making the top links optional. Fixes elastic/uptime#43 * Fix tests to use heartbeat-7 instead of heartbeat-8
* master: (39 commits) [APM]Create custom link from Trace summary (elastic#59648) [ML] Fixing app clean up (elastic#60853) [SIEM] Use ECS categorisation for Authentication widgets (elastic#60734) [NP] Remove kbnUrl usage in discover/dashboard/visualize (elastic#60016) Skip failing test [Uptime]Update fetch effect failed action handling (elastic#60742) [npm] upgrade elastic/maki (elastic#60829) [Uptime] Add Settings Page (elastic#53550) [APM] service maps: avoid unnecesary `useDeepObjectIdentity` (elastic#60836) [Index management] Re-enable index template tests (elastic#60780) Fixed UI/UX issues: alerts delete confirmation, combobox behaviors (elastic#60703) [SIEM] Fix patching of ML Rules (elastic#60830) [APM] Service Map - Separate overlapping edges by rotating nodes (elastic#60477) [Alerting] fix flaky test for index threshold grouping (elastic#60792) [SIEM][Detection Engine] Adds test scripts for machine learning feature Flatten child api response for resolver (elastic#60810) Change "url" to "urls" in APM agent instructions (elastic#60790) [DOCS] Updates API requests and examples (elastic#60695) [SIEM] [Cases] Create case from timeline (elastic#60711) [Lens] Resetting a layer generates new suggestions (elastic#60674) ...
Summary
Adds a settings page to the Uptime UI. The settings page values are per-space. The only current setting is
heartbeatIndices
.To test this against alternate indices try changing
setup.ilm.rollover_alias
inheartbeat.yml
to something likealt-prefix
. See the ilm docs for more details.This should be tested with read-only and write only roles. To test this in kibana try creating two users with two different roles in kibana. One roll should have
read
access to the Uptime space in kibana. The other should haveall
access. Both should haveread
permissions for theheartbeat-*
index pattern.This patch also splits API perms from just
heartbeat
touptime-read
anduptime-write
.This patch also refactors some of the header component functionality, using hooks for breadcrumbs, and making the top links optional.
Fixes elastic/uptime#43
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.For maintainers