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

Jump to bottom

Simplify maybeAddConfig for readability #155137

Closed
afharo opened this issue Apr 18, 2023 · 2 comments · Fixed by #158750
Closed

Simplify maybeAddConfig for readability #155137

afharo opened this issue Apr 18, 2023 · 2 comments · Fixed by #158750
Assignees
@delanni
Labels
Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Operations Team label for Operations Team technical debt Improvement of the software architecture and operational architecture

Comments

@afharo
Copy link
Member

afharo commented Apr 18, 2023 •
edited
Loading

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

Originally posted by @jloleysens in #155009 (comment)
@botelastic botelastic bot added the needs-team Issues missing a team label label Apr 18, 2023
@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 technical debt Improvement of the software architecture and operational architecture labels Apr 18, 2023
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@botelastic botelastic bot removed the needs-team Issues missing a team label label Apr 18, 2023
@afharo afharo mentioned this issue Apr 18, 2023
Merged
1 task
@delanni delanni self-assigned this May 30, 2023
This was referenced May 31, 2023
Closed
Merged
delanni added a commit that referenced this issue Jun 8, 2023
@delanni @kibanamachine
Serve.js refactors (#158750)
f51f5f4 
Closes #155137, with some extra reorganisation, modularisation and unit
tests.

### Refactors to `maybeAddConfig`

### Refactoring serve.js <-> bootstrap.ts

### Unit tests for `compileConfigStack`
---------

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
Assignees

@delanni delanni

Labels
Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Operations Team label for Operations Team technical debt Improvement of the software architecture and operational architecture
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Serve.js refactors delanni/kibana
3 participants
@delanni @afharo @elasticmachine