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

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

Merged
merged 2 commits into from
Apr 18, 2023

Conversation

afharo
Copy link
Member

@afharo afharo commented Apr 17, 2023

Summary

At the moment, we only allow using the --serverless CLI flag in dev mode. However, we want to use it in our serverless deployments. According to #149878, it is a valid option (apparently, there are no big reasons for it to be dev-only).

For maintainers

@afharo afharo added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Operations Team label for Operations Team Project:Serverless Work as part of the Serverless project for its initial release labels Apr 17, 2023
@afharo afharo self-assigned this Apr 17, 2023
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@afharo afharo added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting labels Apr 17, 2023
@afharo afharo requested review from a team April 17, 2023 08:34
Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

Did not test locally, but these changes LGTM! Great work @afharo

Comment on lines 113 to 116
const path = resolve(getConfigDirectory(), name);
if (configFileExists(name)) {
configs[method](path);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
const path = resolve(getConfigDirectory(), name);
if (configFileExists(name)) {
configs[method](path);
}
if (configFileExists(name)) {
const path = resolve(getConfigDirectory(), name);
configs[method](path);
}

or remove path variable entirely.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good one! I applied your suggestion!

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

@afharo afharo enabled auto-merge (squash) April 18, 2023 11:15
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 432 435 +3

Total ESLint disabled count

id before after diff
securitySolution 512 515 +3

History

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

cc @afharo

@afharo afharo merged commit 9d30965 into elastic:main Apr 18, 2023
@afharo afharo deleted the allow-using-serverless-cli-flag branch April 18, 2023 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Project:Serverless Work as part of the Serverless project for its initial release 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 Team:Operations Team label for Operations Team v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants