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

feat(config): allow exporting async config #13075

Merged

Conversation

pmorch
Copy link
Contributor

@pmorch pmorch commented Dec 12, 2021

module.exports can now be a function and it can be/return a Promise,
allowing the results of asynchronous operations to be used in the
configuration.

The discussion leading up to this PR in #13035 assumed that
module.exports had to be a plain object.

But this commit:

  commit 9aa97af5b346f82b48564c20c66783b51c3022d9
  Author: Nejc Habjan <hab.nejc@gmail.com>
  Date:   Thu Dec 9 13:45:48 2021 +0100

      feat(config)!: parse JSON5/YAML self-hosted admin config (#12644)

      Adds support for alternative admin config file formats.

      BREAKING CHANGE: Renovate will now fail if RENOVATE_CONFIG_FILE is specified without a file extension

Had as an undocumented side effect, that it also handled transparenty
if module.exports was assigned a Promise. With that commit, the
promise will be await-ed so the resolved value is returned from
getConfig(). That was not the case before that commit.

So in this commit, configs that export functions are handled, and
test cases for both promises and functions have been added.

Changes:

Context:

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

This would probably be good to document somewhere, but where? I'm happy to add some documentation, if somebody can point me to where the config file is described.

How I've tested my work (please tick one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

module.exports can now be a function and it can be/return a Promise,
allowing the results of asynchronous operations to be used in the
configuration.

The discussion leading up to this PR in renovatebot#13035 assumed that
module.exports had to be a plain object.

But this commit:

  commit 9aa97af
  Author: Nejc Habjan <hab.nejc@gmail.com>
  Date:   Thu Dec 9 13:45:48 2021 +0100

      feat(config)!: parse JSON5/YAML self-hosted admin config (renovatebot#12644)

      Adds support for alternative admin config file formats.

      BREAKING CHANGE: Renovate will now fail if RENOVATE_CONFIG_FILE is specified without a file extension

Had as an undocumented side effect, that it also handled transparenty
if module.exports was assigned a Promise. With that commit, the
promise will be await-ed so the resolved value is returned from
getConfig(). That was not the case before that commit.

So in this commit, configs that export functions are handled, and
test cases for both promises and functions have been added.
@CLAassistant
Copy link

CLAassistant commented Dec 13, 2021

CLA assistant check
All committers have signed the CLA.

@rarkins
Copy link
Collaborator

rarkins commented Dec 13, 2021

Can we document it too?

@pmorch
Copy link
Contributor Author

pmorch commented Dec 13, 2021

Can we document it too?

Sure. But as i wrote, I'd like a pointer as to where to at the documentation. I found the documentation a little confusing in this regard.

@rarkins
Copy link
Collaborator

rarkins commented Dec 13, 2021

@viceice viceice changed the title feat(config): allow exporting async config (#13035) feat(config): allow exporting async config Dec 13, 2021
@@ -5,6 +5,7 @@ import { migrateConfig } from '../../../../config/migration';
import type { AllConfig, RenovateConfig } from '../../../../config/types';
import { logger } from '../../../../logger';
import { readFile } from '../../../../util/fs';
import is from 'is';
Copy link
Member

Choose a reason for hiding this comment

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

Wrong import order. 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll move the import up above the local imports. Are there other rules about import sequence that I should be aware about, or is this good?:

import { load } from 'js-yaml';
import is from 'is';
import JSON5 from 'json5';
import upath from 'upath';
import { migrateConfig } from '../../../../config/migration';
import type { AllConfig, RenovateConfig } from '../../../../config/types';
import { logger } from '../../../../logger';
import { readFile } from '../../../../util/fs';

Copy link
Member

Choose a reason for hiding this comment

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

Run yarn lint. It will tell you all issues, or check the ci status. 😉

Copy link
Collaborator

@HonkingGoose HonkingGoose 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 can probably put the whole comment on one line?

pmorch and others added 2 commits December 13, 2021 13:31
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
…se.js

Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
@pmorch
Copy link
Contributor Author

pmorch commented Dec 13, 2021

Linter also reported a problem with kebab-case in file names. So I'll rename e.g. fileFunction.js to file-function.js and try again.

@pmorch
Copy link
Contributor Author

pmorch commented Dec 13, 2021

I think I've now fixed the issues that have been discussed. A few things to note:

  • I've created a separate commit documenting the changes that were introduced in 9aa97af referencing feat(config)!: parse JSON5/YAML self-hosted admin config #12644.
  • If tried to use my best judgement in creating commit messages for the multiple commits that now exist for this issue. They all have #13035 in the subject line.
    • There were two commits I created from the github UI that didn't get "nice" commit messages. Sorry.
  • I took the liberty of renaming lib/workers/global/config/parse/__fixtures__/file*.js to config*.js, because they really were config files.

If it were just me, I'd rebase and force-push a new commit to fix this issue, so that we get clean history and a single collective commit for this issue. Some are for and some are against force pushing in merge requests. There would then be one commit to document 9aa97af / #12644 and one commit for this issue. Are you for or against that?

Full disclosure: I was not able to run

yarn test

without modifications. I have 12 CPU cores and 16 GB RAM, and jest would try to run 12 parallel tests some of which use more than 3GB RAM each. Eventually my machine would reach load 100, would start swapping like crazy and pretty much die until I hit CTRL-C. I had to introduce --max-workers=4 to run yarn test like this:

diff --git a/package.json b/package.json
index 943619336..64483b8ce 100644
--- a/package.json
+++ b/package.json
@@ -22,7 +22,7 @@
     "generate": "run-s generate:*",
     "generate:imports": "node tools/generate-imports.mjs",
     "git-check": "node tools/check-git-version.mjs",
-    "jest": "cross-env LOG_LEVEL=fatal node --expose-gc node_modules/jest/bin/jest.js --logHeapUsage",
+    "jest": "cross-env LOG_LEVEL=fatal node --expose-gc node_modules/jest/bin/jest.js --logHeapUsage --max-workers=4",
     "jest-debug": "cross-env NODE_OPTIONS=--inspect-brk yarn jest --testTimeout=100000000",
     "jest-silent": "cross-env yarn jest --reporters jest-silent-reporter",
     "lint": "run-s ls-lint eslint prettier markdown-lint git-check doc-fence-check",

It now ran successfully including jest, linter and everything :-)

@viceice
Copy link
Member

viceice commented Dec 13, 2021

you can run yarn jest --max-workers=4 😉 On my maschin i like to use all my 16 cores

@viceice
Copy link
Member

viceice commented Dec 13, 2021

We will squash pr's, so we only have one commit on main 🤗

@viceice viceice linked an issue Dec 13, 2021 that may be closed by this pull request
pmorch and others added 2 commits December 13, 2021 22:30
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
@pmorch
Copy link
Contributor Author

pmorch commented Dec 13, 2021

you can run yarn jest --max-workers=4 😉 On my maschin i like to use all my 16 cores

Yes, this is exactly what I did... Which is also exactly why I forgot to run yarn lint :-)

and I can't run yarn test --max-workers=4 so I had to patch package.json.

@viceice viceice enabled auto-merge (squash) December 13, 2021 21:43
@viceice viceice merged commit c7a7ffb into renovatebot:main Dec 13, 2021
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 31.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Export an async function from config.js
6 participants