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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: provide config extends feature #2222

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

antongolub
Copy link

@antongolub antongolub commented Feb 12, 2024

This PR provides config extensions with github-hosted assets:

{
  "extends": "owner/repo/main",
  "packages": {
    "packages/bot-config-utils": {
    }
  }
}
  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #2048 馃
Fixes #1979

@antongolub antongolub requested review from a team as code owners February 12, 2024 18:19
@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Feb 12, 2024
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Feb 12, 2024
Copy link
Contributor

@chingor13 chingor13 left a comment

Choose a reason for hiding this comment

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

Thanks for this pull request and this general feature seems useful!

However, we're aiming to be conservative with the number of external dependencies we take, and config management hasn't been enough of a pain point that we want to outsource this logic yet.

To me, it seems like we need a way to fetch the remote files (included in this PR) and a deepMerge implementation (~15 LoC, rather than the external dependency).


import * as https from 'https';

// Hmmm... no-experimental-fetch flag is used. Ok
Copy link
Contributor

Choose a reason for hiding this comment

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

This flag is used in test because we've been using nock to fake http requests and it can't handle the built-in fetch :(

Copy link
Author

Choose a reason for hiding this comment

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

Is it necessary to mock the transport layer? Maybe http.getJson mock would suffice here?

@@ -0,0 +1,7 @@
{
"extends": "owner/repo/main",
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps:

  • owner/repo@branchOrSha:path/to/file.json
  • owner/repo/path/to/file.json@branchOrSha
  • owner/repo:path/to/file.json@branchOrSha

Or something similar to https://docs.renovatebot.com/config-presets/#github

Copy link
Author

Choose a reason for hiding this comment

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

We can combine. I've added support for the renovate-like format and for the last two options.

@antongolub
Copy link
Author

antongolub commented Feb 13, 2024

~15 LoC, rather than the external dependency

I understand... But once the 15loc deepmerge meets the prod area, it turns into 115loc deepmerge. And another 1115 lines of tests. I know this road; I have walked along it many many times. I'd even suggest using yargs/helpers#applyExtends here, but unfortunately there is no way to override its internal loader.

Dependencies are always a trade-off between complexity, risks and implementation costs. For example, someone's reinventing cosmiconfig in place (ref1, ref2, ref3), while release-please uses yaml@^2.2.2 and js-yaml@^4.0.0 as prod deps as the same time :)

deepmerge itself doesn't answer what to do with plugins: concat or replace?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FR: pull from external config Could I extend a universal configuration?
2 participants