-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Reporting] Switch Serverside Config Wrapper to NP #62500
[Reporting] Switch Serverside Config Wrapper to NP #62500
Conversation
a189e82
to
62022cf
Compare
@@ -12,8 +12,6 @@ import { createQueueFactory, enqueueJobFactory, LevelLogger, runValidations } fr | |||
import { setFieldFormats } from './services'; | |||
import { ReportingSetup, ReportingSetupDeps, ReportingStart, ReportingStartDeps } from './types'; | |||
import { registerReportingUsageCollector } from './usage'; | |||
// @ts-ignore no module definition | |||
import { mirrorPluginStatus } from '../../../server/lib/mirror_plugin_status'; |
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.
ts-ignore
was hiding this dead code
ed4ea89
to
b887cec
Compare
b887cec
to
886f976
Compare
@@ -204,11 +78,12 @@ export const buildConfig = ( | |||
}; | |||
|
|||
// spreading arguments as an array allows the return type to be known by the compiler | |||
reportingConfig = addConfigDefaults(server, core, reportingConfig); |
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.
Config defaults are now being applied in NP code in createConfig$
@elasticmachine merge upstream |
public async setup(core: CoreSetup): Promise<PluginsSetup> { | ||
return { | ||
__legacy: { | ||
config$: createConfig$(core, this.initializerContext, this.log).pipe(first()), |
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.
For now, the only role of the Reporting server-side plugin in NP is to expose the config in the plugin contract.
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.
LGTM!
@elasticmachine merge upstream |
@@ -10,7 +10,7 @@ type ViewportConfig = CaptureConfig['viewport']; | |||
type BrowserConfig = CaptureConfig['browser']['chromium']; | |||
|
|||
interface LaunchArgs { | |||
userDataDir: BrowserConfig['userDataDir']; | |||
userDataDir: string; |
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.
this was a bug with pre-existing type defs
|
||
interface BrowserConfig { | ||
inspect: boolean; | ||
userDataDir: string; |
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.
userDataDir
is actually not in the reporting plugin config
@@ -11,7 +11,7 @@ import { PluginStart as DataPluginStart } from '../../../../../src/plugins/data/ | |||
import { SecurityPluginSetup } from '../../../../plugins/security/server'; | |||
import { XPackMainPlugin } from '../../xpack_main/server/xpack_main'; | |||
import { ReportingPluginSpecOptions } from '../types'; | |||
import { ReportingConfig, ReportingConfigType } from './core'; | |||
import { ReportingConfigType } from './core'; |
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.
cleaned up unused import
@elasticmachine merge upstream |
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.
Few small questions on the PR but otherwise lgtm
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* New config * fix translations json * add csv.useByteOrderMarkEncoding to schema * imports cleanup * restore "get default chromium sandbox disabled" functionality * integrate getDefaultChromiumSandboxDisabled * fix tests * --wip-- [skip ci] * add more schema tests * diff prettiness * trash legacy files that moved to NP * create_config tests * Hoist create_config * better disableSandbox tests * fix ts * fix export * fix bad code * make comments better * fix i18n * comment * automatically setting... logs * replace log_configuration * fix lint * This is f2 * improve startup log about sandbox info * update docs with log reference * revert log removal Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
* New config * fix translations json * add csv.useByteOrderMarkEncoding to schema * imports cleanup * restore "get default chromium sandbox disabled" functionality * integrate getDefaultChromiumSandboxDisabled * fix tests * --wip-- [skip ci] * add more schema tests * diff prettiness * trash legacy files that moved to NP * create_config tests * Hoist create_config * better disableSandbox tests * fix ts * fix export * fix bad code * make comments better * fix i18n * comment * automatically setting... logs * replace log_configuration * fix lint * This is f2 * improve startup log about sandbox info * update docs with log reference * revert log removal Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
* master: (56 commits) [i18n] Update CODEOWNERS (elastic#63354) add platform team definition of done (elastic#59993) [SIEM] move away from Joi for importing/exporting timeline (elastic#62125) Fix discover preserve url (elastic#63580) [alerting] Adds an alertServices mock and uses it in siem, monitoring and uptime (elastic#63489) Closes elastic#63109 for Service Map by resetting edges styles for the selected node (elastic#63655) MIgrated index_header to react (elastic#63490) Index pattern management UI -> TypeScript and New Platform Ready (indexed_fields_table) (elastic#63364) [SIEM] [Cases] Insert timeline and reporters/tags in table bug fixes (elastic#63642) [Reporting] Make usable default element positions (elastic#63191) [Reporting] Switch Serverside Config Wrapper to NP (elastic#62500) [Reporting] Add "warning" status as an alternate type of completed job (elastic#63498) Split action types into own page (elastic#63516) [Lens] Only show copy on save for previously saved docs (elastic#63535) Update README.md (elastic#63622) Bugfix clear saved query crashes kibana on Discover in some cases (elastic#63554) Add uptime CODEOWNER entries. (elastic#63616) [ML] Extract apiDoc params from the schema definitions (elastic#62933) Fix alerting documentation encryption key requirement (elastic#63512) Fix CODEOWNERS and sass lint paths (elastic#63552) ...
Summary
Followup of: #62086
Replaces: #61696
#62086 shimmed the server code with a module that abstracts reading config settings in Reporting. This PR switches the internals of the shim to source the config from NP instead of legacy.
Checklist
Delete any items that are not applicable to this PR.
For maintainers