Serve.js refactors #158750

delanni · 2023-05-31T16:36:52Z

Summary

Closes #155137, with some extra reorganisation, modularisation and unit tests.

Refactors to `maybeAddConfig`

Mostly in (b8fa2a0)

Refactoring serve.js <-> bootstrap.ts

Previously, both of these files were involved with assessing wether a use-case is a serverless case or not. serve.js would assess if the runtime options point to a serverless run. If so, it would write a flag to a config file, and add it to the config stack. bootstrap.ts would read the config stack, assess if ultimately it's a case of serverless run (either through the options and the written file, or through any other config file containing the serverless setting), then optionally extend the config stack, and reload the configs. It seemed that the concern of figuring out what's the final configuration stack was split across two files, and that's why 'hacks' were needed.

My attempt was to compartmentalise the code for figuring out what's the final configuration stack (the list of configs that would be merged) within serve.js, so that:

serve.js alone can decide on the final stack of configs
bootstrap.ts can be agnostic of the config stack, just receive it, and use it
this also allows for modularising the compiling of the config set and order, and allowing for unit tests

Unit tests for `compileConfigStack`

This replaces integration tests around serve.js and bootstrap.ts that examine config lists w/r/t serverless.

Checklist

afharo

I love that we are simplifying (or, at least, bringing together the handling of the config files), and I'd love to have this merged.

However, I think that we are breaking the current Project Switcher's functionality. Refer to this comment for the expected flow.

afharo · 2023-06-01T07:43:40Z

src/cli/serve/serve.js

+      configs.unshift(resolveConfig(`serverless.${serverlessMode}.yml`));
+      configs.unshift(resolveConfig('serverless.yml'));
+
+      if (opts.dev) {


nit: I wonder if we should check opts.devConfig !== false here as well

It's a fair question. I don't know how it should work, or what the use of the --no-dev-config flag could be in practise. I'm happy to extend this condition, and include it in the help-text too.

IMO, we should remove serverless.dev.yml (I don't fully get what's its purpose), but that's unrelated to this PR.

If we did that, I think it's fine the way it is right now (no need for the ops.devConfig check.

afharo · 2023-06-01T07:46:24Z

packages/core/root/core-root-server-internal/src/bootstrap.ts

-  // Hack to load the extra serverless config files if `serverless: {projectType}` is found in it.
-  const rawConfig = await firstValueFrom(rawConfigService.getConfig$());
-  const serverlessProjectType = rawConfig?.serverless;
-  if (
-    typeof serverlessProjectType === 'string' &&
-    VALID_SERVERLESS_PROJECT_TYPES.includes(serverlessProjectType)
-  ) {
-    const extendedConfigs = [
-      ...['serverless.yml', `serverless.${serverlessProjectType}.yml`]
-        .map((name) => resolve(getConfigDirectory(), name))
-        .filter(configFileExists),
-      ...configs,
-    ];
-
-    env = Env.createDefault(REPO_ROOT, {
-      configs: extendedConfigs,
-      cliArgs: { ...cliArgs, serverless: true },
-      repoPackages: getPackages(REPO_ROOT),
-    });
-
-    rawConfigService.stop();
-    rawConfigService = new RawConfigService(env.configs, applyConfigOverrides);
-    rawConfigService.loadConfig();
-  }
-


Thank you for moving this back to the serve.js file 🧡

afharo · 2023-06-01T07:49:32Z

src/cli/serve/serve.js

+      if (opts.dev) {
+        configs.push(resolveConfig('serverless.dev.yml'));
+
+        writeProjectSwitcherConfig('serverless.recent.dev.yml');


I love that we renamed serverless.recent.yml to serverless.recent.dev.yml. It makes it more obvious that this file won't exist in production, and that it's a dev utility.

Just remember to change it here as well. A potential solution could be to add the path to the file in the config so the route always knows where it lives.

src/cli/serve/serve.js

afharo · 2023-06-01T08:15:30Z

src/cli/serve/serve.js

+        configs.push(resolveConfig('serverless.dev.yml'));
+
+        writeProjectSwitcherConfig('serverless.recent.dev.yml');
+        configs.push(resolveConfig('serverless.recent.dev.yml'));


IMO, we should only add this file if --serverless is true. With the changes proposed in this PR, for explicit project types, it shouldn't be used.

afharo · 2023-06-01T08:16:17Z

src/cli/serve/serve.js

+    // Filtering out all config paths that didn't exist
+    configs = configs.filter(Boolean);
+
+    const serverlessMode = getServerlessModeFromOpts(opts) || getServerlessModeFromCfg(configs);


IMO, this check needs to be more complex:

If we keep it like this, it'll break the project switcher's dev functionality. Typical flow:

Dev starts Kibana with yarn start --serverless (no project specified)

The first time, Project Switcher defaults to es ✅. Otherwise, it checks the latest project type it was running (it was stored in serverless.recent.yml) 🛑 with this approach, opts.serverless === true will always return es.

While running Kibana, the developer could switch the project type. POST /internal/serverless/switch_project would write the selected project type in serverless.recent.yml, and the watcher would trigger a Kibana restart. serve.js reads this new config and starts Kibana in the new project type.

Of course, this logic applies only if the CLI option --serverless is true (no explicit project selected).

cc @clintandrewhall as the original author of Project Switcher in case I missed any feature 😇

Understood! This was an oversight from my side, I automatically placed the concepts together (serverless => project switcher). I'll iron this out in a minute!

@afharo - according to the original logic, the most recent --serverless choice is also persisted in the file anytime it's a string. I'll adjust my code to reflect this behaviour

delanni · 2023-06-05T09:33:43Z

@elasticmachine merge upstream

elasticmachine · 2023-06-05T09:34:14Z

Pinging @elastic/kibana-operations (Team:Operations)

… in bootstrap.ts

delanni · 2023-06-06T14:57:06Z

@afharo / @elastic/kibana-core - this is now ready for review, I think.

One more thing I could easily add: should I rewrite the new component to typescript? It's always hard to tell whether I should change code to Typescript next to other sources written in Javascript, but this one would be a nobrainer.

afharo

I haven't tested it. But code changes LGTM.

afharo · 2023-06-06T14:59:17Z

packages/core/root/core-root-server-internal/src/bootstrap.ts

  const root = new Root(rawConfigService, env, onRootShutdown);
  const cliLogger = root.logger.get('cli');

-  cliLogger.debug('Kibana configurations evaluated in this order: ' + env.configs.join(', '));


Q: should we keep this?

I could add it back, at a debug level, it's probably useful

Yes, please! I'm sure it'll save us from some headaches in the future :)

afharo · 2023-06-06T15:00:35Z

src/cli/serve/compile_config_stack.js

+    configs.push(resolveConfig('kibana.dev.yml'));
+  }
+
+  if (dev && serverless) {


nit: we only need the recent file when the serverless flag didn't specify a project

Suggested change

if (dev && serverless) {

if (dev && serverless === true) {

I'm aware that's not the current behaviour. Happy if we change it in a follow-up PR.

I'll probably not want to change behaviour in this PR, to keep it as a refactor, and not to have to explain myself if anyone asks me :-)

++ no need to change it in this PR. Let's keep the scope of this PR to a refactor :)

afharo · 2023-06-06T15:09:32Z

src/cli/serve/compile_config_stack.js

+    if (dev && devConfig !== false) {
+      configs.push(resolveConfig('serverless.dev.yml'));
+      configs.push(resolveConfig(`serverless.${serverlessMode}.dev.yml`));
+    }


TBH, I don't see the value of these files...

serverless.yml and serverless.${serverlessMode}.yml files are supposed to set the defaults for the Serverless offering. Having .dev files that override them for dev feels like something's wrong about our defaults (especially, since we already have kibana.dev.yml for things like pointing a service to a local service.

However, I wouldn't consider this a blocker for this PR.

cc @lukeelmers, WDYT?

Spencer's original PR had support for these project-specific .dev.yml files which I think got lost in a later update:

kibana/config/README.md

Lines 7 to 16 in f48b742

configuration is applied in the following order, later values override

1. serverless.yml (serverless configs go first)

2. serverless.{mode}.yml (serverless configs go first)

3. base config, in this preference order:

- my-config.yml(s) (set by --config)

- env-config.yml (described by `env.KBN_CONFIG_PATHS`)

- kibana.yml (default @ `env.KBN_PATH_CONF`/kibana.yml)

4. kibana.dev.yml

5. serverless.dev.yml

6. serverless.{mode}.dev.yml

I don't have a strong feeling on this. Since kibana.dev.yml always wins, I don't think there's a functional need for the project-specific dev overrides. It's just more of an organizational question... If folks want to override a security-specific setting, for example, it may be clearer to have the ability to copy that into a designated file that they could easily map back to the original security serverless yml.

It's a dev-only feature, so if there's anyone who would find it useful and it doesn't cost us much to maintain, then I don't see any harm in keeping it.

I'm easily swayed towards either behaviour, let me know which one you prefer (keep the specific configs, or only have kibana.dev.yml)

It's a dev-only feature, so if there's anyone who would find it useful and it doesn't cost us much to maintain, then I don't see any harm in keeping it.

My main concern is that a dev perceives a different behaviour just because they are using those serverless.*dev.yml files and forgot about them.

Having said that, I don't think we need to increase the scope of this PR. We can discuss it, and remove those later if needed.

afharo · 2023-06-06T16:02:12Z

src/cli/serve/compile_config_stack.js

+  if (serverlessMode === true) {
+    // Defaulting to read the project-switcher's settings in `serverless.recent.dev.yml`
+    return null;
+  }


delanni · 2023-06-07T09:59:22Z

x-pack/plugins/serverless/server/plugin.ts

@@ -63,7 +63,7 @@ export class ServerlessPlugin implements Plugin<ServerlessPluginSetup, Serverles
            // with a specific config.  So in this case, to ensure the switcher remains enabled,
            // write the selected config to `recent` and tack on the setting to enable the switcher.
            writeFileSync(
-              resolve(getConfigDirectory(), 'serverless.recent.yml'),


@elastic/appex-sharedux - could you check if you're okay with this small change. We changed the name of the generated config file to serverless.recent.dev.yml

kibana-ci · 2023-06-07T10:57:16Z

💚 Build Succeeded

Buildkite Build
Commit: 262b133

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`securitySolution`	414	418	+4
total			+6

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`securitySolution`	498	502	+4
total			+6

History

💚 Build #132605 succeeded 838f6dd
💔 Build #132560 failed 5415ba7
💔 Build #132472 failed 9442e86
💔 Build #132193 failed 03cc44d
💔 Build #132138 failed 3c80e79
💔 Build #132032 failed 72fb392

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

tsullivan

serverless plugin changes LGTM

Prior to #158750, `bin/kibana --serverless=<project>` was able to load project specific configurations on Kibana distributions. This was used in functional tests [here](https://github.com/elastic/kibana/blob/main/x-pack/test_serverless/functional/config.base.ts#L31), but admittedly may not be the correct way to start the Kibana server. Opening this up for discussion 1) Do we want to support `bin/kibana --serverless`? 2) What's the recommended way to run a distribution in serverless mode? --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>

Prior to elastic#158750, `bin/kibana --serverless=<project>` was able to load project specific configurations on Kibana distributions. This was used in functional tests [here](https://github.com/elastic/kibana/blob/main/x-pack/test_serverless/functional/config.base.ts#L31), but admittedly may not be the correct way to start the Kibana server. Opening this up for discussion 1) Do we want to support `bin/kibana --serverless`? 2) What's the recommended way to run a distribution in serverless mode? --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>

chore: Tidy up serve.js

b8fa2a0

delanni requested a review from a team as a code owner May 31, 2023 16:36

delanni force-pushed the further-serve-refactors branch from 88b13ce to 0cc569a Compare May 31, 2023 16:49

refactor: move config evaluation for serverless to serve.js

b9cb27a

delanni force-pushed the further-serve-refactors branch from 0cc569a to b9cb27a Compare May 31, 2023 20:52

afharo reviewed Jun 1, 2023

View reviewed changes

delanni added 2 commits June 1, 2023 16:55

fix: Fix regression regarding configuration stack ordering

f75a40d

test: add integration test for testing config parse order

72fb392

delanni mentioned this pull request Jun 2, 2023

Fix config stacking order #158827

Merged

1 task

delanni added backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes labels Jun 2, 2023

delanni marked this pull request as draft June 2, 2023 09:20

delanni added the Team:Operations Team label for Operations Team label Jun 2, 2023

delanni mentioned this pull request Jun 2, 2023

Tidy up serve.js #158706

Closed

1 task

delanni changed the title ~~Further serve refactors~~ Serve.js refactors Jun 2, 2023

delanni added 3 commits June 2, 2023 16:46

refactor: re-create project switcher logic, address PR comments

3c80e79

test: increase integration test timeout, mock logger functions

03cc44d

refactor: move config ordering to a module, unit test > integration test

bae59da

Merge branch 'main' into further-serve-refactors

9442e86

delanni marked this pull request as ready for review June 5, 2023 09:34

delanni requested a review from a team as a code owner June 5, 2023 09:34

delanni added 4 commits June 5, 2023 13:09

test: remove config-reloading unit-tests, as it's no longer happening…

8eff62a

… in bootstrap.ts

refactor: remove log line that was for integration tests

570f866

Merge branch 'main' into further-serve-refactors

5415ba7

test: fix typo in parameter passing

838f6dd

afharo approved these changes Jun 6, 2023

View reviewed changes

refactor: re-add debug line about config order

262b133

delanni commented Jun 7, 2023

View reviewed changes

tsullivan approved these changes Jun 7, 2023

View reviewed changes

delanni merged commit f51f5f4 into elastic:main Jun 8, 2023
16 checks passed

kibanamachine added the v8.9.0 label Jun 8, 2023

jbudz mentioned this pull request Jun 12, 2023

[cli] Re-add --serverless support on distributions #159516

Merged

delanni deleted the further-serve-refactors branch May 2, 2024 11:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serve.js refactors #158750

Serve.js refactors #158750

delanni commented May 31, 2023 •

edited

afharo left a comment •

edited

afharo Jun 1, 2023

delanni Jun 1, 2023

afharo Jun 1, 2023

afharo Jun 1, 2023

afharo Jun 1, 2023

afharo Jun 1, 2023

afharo Jun 1, 2023

delanni Jun 1, 2023

delanni Jun 2, 2023

delanni commented Jun 5, 2023

elasticmachine commented Jun 5, 2023

delanni commented Jun 6, 2023

afharo left a comment

afharo Jun 6, 2023

delanni Jun 7, 2023

afharo Jun 7, 2023

afharo Jun 6, 2023

delanni Jun 7, 2023

afharo Jun 7, 2023

afharo Jun 6, 2023

lukeelmers Jun 6, 2023

delanni Jun 7, 2023

afharo Jun 7, 2023

afharo Jun 6, 2023

delanni Jun 7, 2023

kibana-ci commented Jun 7, 2023

ESLint disabled line counts

Total ESLint disabled count

tsullivan left a comment

	configuration is applied in the following order, later values override
	1. serverless.yml (serverless configs go first)
	2. serverless.{mode}.yml (serverless configs go first)
	3. base config, in this preference order:
	- my-config.yml(s) (set by --config)
	- env-config.yml (described by `env.KBN_CONFIG_PATHS`)
	- kibana.yml (default @ `env.KBN_PATH_CONF`/kibana.yml)
	4. kibana.dev.yml
	5. serverless.dev.yml
	6. serverless.{mode}.dev.yml

Serve.js refactors #158750

Serve.js refactors #158750

Conversation

delanni commented May 31, 2023 • edited

Summary

Refactors to maybeAddConfig

Refactoring serve.js <-> bootstrap.ts

Unit tests for compileConfigStack

Checklist

afharo left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delanni commented Jun 5, 2023

elasticmachine commented Jun 5, 2023

delanni commented Jun 6, 2023

afharo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Jun 7, 2023

💚 Build Succeeded

Metrics [docs]

ESLint disabled line counts

Total ESLint disabled count

History

tsullivan left a comment

Choose a reason for hiding this comment

delanni commented May 31, 2023 •

edited

Refactors to `maybeAddConfig`

Unit tests for `compileConfigStack`

afharo left a comment •

edited