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.

Sign up for GitHub

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

Allow using the --serverless CLI flag in serverless-capable distros #155009

Merged
merged 2 commits into from
Apr 18, 2023
Merged

Allow using the --serverless CLI flag in serverless-capable distros #155009

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3172a9a
Allow using the `--serverless` CLI flag in serverless-capable distrib…
afharo Apr 17, 2023
b44fde7
Address PR feedback
afharo Apr 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 31 additions & 10 deletions src/cli/serve/serve.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,25 +78,43 @@ const configPathCollector = pathCollector();
const pluginPathCollector = pathCollector();

/**
* @param {string} name
* @param {string[]} configs
* @param {'push' | 'unshift'} method
* @param {string} name The config file name
* @returns {boolean} Whether the file exists
*/
function maybeAddConfig(name, configs, method) {
function configFileExists(name) {
const path = resolve(getConfigDirectory(), name);
try {
if (statSync(path).isFile()) {
configs[method](path);
}
return statSync(path).isFile();
} catch (err) {
if (err.code === 'ENOENT') {
return;
return false;
}

throw err;
}
}

/**
* @returns {boolean} Whether the distribution can run in Serverless mode
*/
function isServerlessCapableDistribution() {
// For now, checking if the `serverless.yml` config file exists should be enough
// We could also check the following as well, but I don't think it's necessary:
// VALID_SERVERLESS_PROJECT_MODE.some((projectType) => configFileExists(`serverless.${projectType}.yml`))
return configFileExists('serverless.yml');
}

/**
* @param {string} name
* @param {string[]} configs
* @param {'push' | 'unshift'} method
*/
function maybeAddConfig(name, configs, method) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nitnit: I know this is a pre-existing function, but I don't find the API very readable

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

IMO something like this would be clearer:

    if (serverlessMode) {
      configs.push(resolveConfig(`serverless.yml`));
      // we "unshift" .serverless. config so that it only overrides defaults
      configs.unshift(resolveConfig(`serverless.${serverlessMode}.yml`));
    }
    ...
    configs = configs.filter(Boolean);

where resolveConfig: (config: string) => undefined | string

Copy link
Member Author

Choose a reason for hiding this comment

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

++ It looks much cleaner your way!

I've created #155137 to tackle it as an entity change on its own. I'll address it as soon as this one is merged

if (configFileExists(name)) {
configs[method](resolve(getConfigDirectory(), name));
}
}

/**
* @returns {string[]}
*/
Expand Down Expand Up @@ -233,8 +251,11 @@ export default function (program) {
.option(
'--run-examples',
'Adds plugin paths for all the Kibana example plugins and runs with no base path'
)
.option('--serverless <oblt|security|es>', 'Start Kibana in a serverless project mode');
);
}

if (isServerlessCapableDistribution()) {
command.option('--serverless <oblt|security|es>', 'Start Kibana in a serverless project mode');
}

if (DEV_MODE_SUPPORTED) {
Expand Down