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.

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

[Serverless] Select project type via config #155754

Merged
merged 15 commits into from
Apr 27, 2023

Conversation

afharo
Copy link
Member

@afharo afharo commented Apr 25, 2023

Summary

It adds the serverless: <es>|<oblt>|<security> config parameter so it can be set in the kibana.yml file.

This is a requirement by CP (cc @pebrc @sebgl) instead of relying on the existing CLI flag.

I went with serverless as the config parameter so we can remove the CLI flag and still work as expected (options can be provided to Kibana via CLI if they are provided like --{config.key}=value). The way the configuration schema is defined, we can iterate into more complex structures in the future if needed while maintaining BWC.

Note: Due to the Project Type switcher helper, we need to keep the CLI flag. For non-distributable versions of Kibana (running from source), the CLI flag will intercept the config, then write the config serverless in the file config/serverless.recent.yml and add the file to the list of files to read.
In distributable versions of Kibana, the CLI flag won't intercept --serverless, and it will be treated as a config parameter.

For maintainers

@afharo afharo added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc technical debt Improvement of the software architecture and operational architecture release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Project:Serverless MVP labels Apr 25, 2023
@afharo afharo self-assigned this Apr 25, 2023
@afharo afharo requested a review from a team as a code owner April 25, 2023 17:13
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@@ -0,0 +1 @@
uiSettings.overrides.defaultRoute: /app/enterprise_search/content/search_indices
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this setting to change the landing page. This would make it super obvious that we are loading different project type files.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in the PoC, we'll be doing this for IC2, so this works.

@afharo afharo force-pushed the serverless/select-project-type-via-config branch from 539c971 to 8731670 Compare April 25, 2023 17:23
Copy link
Contributor

@rudolf rudolf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'd need to cleanup the existing --serverless cli option and add a test (could be similar to src/cli/serve/integration_tests/reload_logging_config.test.ts. ?) but the approach looks good to me 👍

@afharo
Copy link
Member Author

afharo commented Apr 25, 2023

Thank you @rudolf. I didn't remove the existing CLI option because I wanted to confirm this approach made sense.

I'll push the changes tomorrow morning.

@afharo afharo enabled auto-merge (squash) April 26, 2023 11:56

env = Env.createDefault(REPO_ROOT, {
configs: extendedConfigs,
cliArgs: { ...cliArgs, serverless: true },
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding this for BWC. I noticed we are leveraging it in #155142.

IMO, we should either change Env to expose mode.serverless: true (instead of referring to the cliArgs), or, even better: expose the behavior under configuration that can be set in serverless.yml.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created #155912 to tackle that

@afharo afharo requested a review from a team as a code owner April 26, 2023 21:25
@afharo afharo force-pushed the serverless/select-project-type-via-config branch from 4817761 to b0d33db Compare April 26, 2023 21:32
Copy link
Member Author

@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.

@clintandrewhall, I had to make some changes to the way the projectSwitcher works.

Please, let me know if this is fine.

@@ -124,17 +114,24 @@ function maybeAddConfig(name, configs, method) {
function maybeSetRecentConfig(file, mode, configs, method) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed this method's behaviour to rewrite serverless.recent.yml only to include the settings

xpack.serverless.plugin.developer.projectSwitcher.enabled: true
serverless: ${selectedProjectType}

@clintandrewhall, do you think this is a good approach?

)
.option(
'--serverless <oblt|security|es>',
'Start Kibana in a specific serverless project mode'
'--serverless [oblt|security|es]',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the plain --serverless option was handling all commands, and we were never making it to the values here.

Looking at commander's documentation, using [value] allows us to specify that they can be optional.

@@ -322,18 +312,13 @@ export default function (program) {
const configs = [getConfigPath(), ...getEnvConfigs(), ...(opts.config || [])];
const serverlessMode = getServerlessProjectMode(opts);

// we "unshift" .serverless. config so that it only overrides defaults
if (serverlessMode) {
maybeAddConfig(`serverless.yml`, configs, 'push');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm intentionally not adding serverless.yml because it's added later on when bootstrap.ts finds out in the config that a specific serverless mode is required.

}

// .dev. configs are "pushed" so that they override all other config files
if (opts.dev && opts.devConfig !== false) {
maybeAddConfig('kibana.dev.yml', configs, 'push');
if (serverlessMode) {
maybeAddConfig(`serverless.dev.yml`, configs, 'push');
maybeSetRecentConfig('serverless.recent.dev.yml', serverlessMode, configs, 'unshift');
maybeAddConfig('serverless.recent.dev.yml', configs, 'push');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using maybeAddConfig because otherwise serverless.recent.dev.yml and serverless.recent.yml will be the exact copies. IIUC, this file is used to manually override something in dev mode.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome catch.

);
}

if (isServerlessCapableDistribution()) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Back to --serverless only being available for non-distributables (mind the if(!isKibanaDistributable()) a few lines above.

Comment on lines +65 to +68
writeFileSync(
resolve(getConfigDirectory(), 'serverless.recent.yml'),
`xpack.serverless.plugin.developer.projectSwitcher.enabled: true\nserverless: ${selectedProjectType}\n`
);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as in serve.js: the behavior now is to set serverless: {myChosenProjectType} and enabling the projectSwitcher.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@afharo afharo force-pushed the serverless/select-project-type-via-config branch from b0d33db to bf22c4c Compare April 26, 2023 21:50
@afharo afharo force-pushed the serverless/select-project-type-via-config branch from 1f13e64 to 908bcfa Compare April 26, 2023 22:48
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 17 19 +2
securitySolution 399 402 +3
total +5

Total ESLint disabled count

id before after diff
enterpriseSearch 18 20 +2
securitySolution 479 482 +3
total +5

History

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

cc @afharo

Copy link
Contributor

@clintandrewhall clintandrewhall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally, and we're in good shape. Fantastic compromise, and awesome improvements, too! 🚢

}

// .dev. configs are "pushed" so that they override all other config files
if (opts.dev && opts.devConfig !== false) {
maybeAddConfig('kibana.dev.yml', configs, 'push');
if (serverlessMode) {
maybeAddConfig(`serverless.dev.yml`, configs, 'push');
maybeSetRecentConfig('serverless.recent.dev.yml', serverlessMode, configs, 'unshift');
maybeAddConfig('serverless.recent.dev.yml', configs, 'push');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome catch.

Comment on lines +65 to +68
writeFileSync(
resolve(getConfigDirectory(), 'serverless.recent.yml'),
`xpack.serverless.plugin.developer.projectSwitcher.enabled: true\nserverless: ${selectedProjectType}\n`
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall approach makes sense to me. The one piece of functionality I think we lost is the ability to have a serverless.{mode}.dev.yml which can be merged in as an override similar to kibana.dev.yml. Not sure if that happened in this PR or maybe #155582, but I'm okay addressing in a follow up as this PR is time-sensitive.

@afharo afharo merged commit de64ff5 into elastic:main Apr 27, 2023
@afharo afharo deleted the serverless/select-project-type-via-config branch April 27, 2023 04:49
@afharo afharo added backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) and removed backport:skip This commit does not require backporting labels Apr 27, 2023
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.8 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 155754

Questions ?

Please refer to the Backport tool documentation

afharo added a commit to afharo/kibana that referenced this pull request Apr 27, 2023
(cherry picked from commit de64ff5)

# Conflicts:
#	config/serverless.es.yml
#	config/serverless.security.yml
#	config/serverless.yml
#	src/cli/serve/serve.js
#	x-pack/plugins/serverless/server/plugin.ts
@afharo
Copy link
Member Author

afharo commented Apr 27, 2023

💚 All backports created successfully

Status Branch Result
8.8

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

afharo added a commit that referenced this pull request Apr 27, 2023
# Backport

This will backport the following commits from `main` to `8.8`:
- [[Serverless] Select project type via config
(#155754)](#155754)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Alejandro Fernández
Haro","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-04-27T04:49:44Z","message":"[Serverless]
Select project type via config
(#155754)","sha":"de64ff5edfc2637c042d800f7c4c62d104f35320","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Core","technical
debt","release_note:skip","backport:prev-minor","v8.9.0","Project:Serverless
MVP"],"number":155754,"url":"https://github.com/elastic/kibana/pull/155754","mergeCommit":{"message":"[Serverless]
Select project type via config
(#155754)","sha":"de64ff5edfc2637c042d800f7c4c62d104f35320"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/155754","number":155754,"mergeCommit":{"message":"[Serverless]
Select project type via config
(#155754)","sha":"de64ff5edfc2637c042d800f7c4c62d104f35320"}}]}]
BACKPORT-->

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Project:Serverless MVP release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc technical debt Improvement of the software architecture and operational architecture v8.8.0 v8.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants