Enable config migration by default

[HonkingGoose]

What would you like Renovate to be able to do?

Right now users can opt-in to get config migration PRs by setting configMigration=true. I propose we make getting config migration PRs the default behavior for all users.

Benefits:

• Config files match documentation, which prevents confusion
• People learn about the renamed features at once, when the config migration PR lands

Drawbacks:

• Once we switch the default setting, Renovate creates many config migration PRs, which will be perceived as noise by our users

Opting out by users

Users can opt-out of this new feature by setting configMigration=false in their repository's renovate.json. Organizations can opt-out via their global .github/renovate repository.

If you have any ideas on how this should be implemented, please tell us here.

Maybe add configMigration=true to our config:recommended and config:best-practice presets? Or maybe you want to hard-code this in deeper in the code?

Related issues

We may want to finish these before making config migration the default behavior:

• On-demand config migration PRs using Dashboard #19783
• Migrate deprecated internal presets #7572
• Fix "migrateConfig" argument types #9611
• Refactor migrations #11457
• Migrate inline with same sort  #11459
• Config migration PR title doesn't follow Conventional Commits spec #16364
• fix(config): preserve packageRules order when migrating #16367
• fix(config): don't skip migration PR if closed PR found #16380
• Reduce config migration whitespace changes for arrays #16383
• refactor(migrations): class-based migration for packageRules #16390
• refactor(migrations): migrate package files #16393
• refactor(migrations): master issue #16399
• Config migration: run prettier #16402
• refactor(migrations): migrate depTypes #16421
• feat: run prettier on migratedJSON #16471
• Move config migration PR creation to the main repository worker flow #16736
• Config migration ignores closed PR and creates a new one #16774
• Adjust config migration PR title to not show filenames #16780
• Config migration - move applyPrettierFormatting to later in code #17336
• Config Migration branch should be based from the default branch #17534
• Show user if config migration is needed for their own config or for an extends #21456
• Test the configMigration feature on our own repositories, make sure it's good and bug-free
• Ideally all migration refactors are done

Is this a feature you are interested in implementing yourself?

No

[rarkins]

I think we should first try to get more use of this feature by prompting people in the dependency dashboard that to open the migration PR "on demand": #19783

[rarkins]

I don't think this is ready until we can reduce the amount of unnecessary formatting changes, e.g.

https://github.com/renovate-reproductions/migrate1/pull/2/files

The amount of green/red makes it hard to see the actual changes

[rarkins]

The good news is that adding a .prettierrc to that repository resulted in those changes mostly disappearing.

Bad news is that doesn't fix for non-prettier cases

[viceice]

The good news is that adding a .prettierrc to that repository resulted in those changes mostly disappearing.

Bad news is that doesn't fix for non-prettier cases

fixing that without knowledge about the current formatting (like prettier) is pretty much impossible.

[voxpelli]

fixing that without knowledge about the current formatting (like prettier) is pretty much impossible.

You would need to parse the JSON while persisting formatting data, you could maybe use tree-sitter-json for that or do some quick parsing yourself based on eg. some peg-style grammars out there, considering that JSOn is quite simple to parse. I did a quick rough incomplete such sketch here: https://gist.github.com/voxpelli/3af3a377ed13c2ffa7e507437974fd2b

[rarkins]

Another idea: parse the git diff output, then compare chunk by chunk. Parse each removed/added chunk and if they are equal then revert that change. A challenge would be where multiple changes are adjacent because then git groups them together.

[TWiStErRob]

I don't think this is ready until we can reduce the amount of unnecessary formatting changes

I would think that's acceptable on the first enable of the migration, and after that the formatting and migrations would be kept up to date by smaller trivial PRs.

---

That said, I would love to option to keep my JSON formatting style and if adding a config for it is the way, then be it, but I wouldn't want it to affect JSON files other than than Renovate config, if that's possible.

I like @voxpelli's approach, clean and targeted. Keep in mind JSON5 and other formats though.

[TWiStErRob]

@rarkins would adding the prettierrc temporarily in the clone, without actually committing it, work?

[voxpelli]

There's this parser from @ota-meshi that supports JSON5, JSONC and plain JSON: https://github.com/ota-meshi/jsonc-eslint-parser

Though it seems to currently only do parsing, not serializing.

Ideally there would be a JSON Patch library built on top of such a white-space preserved AST and then a serialization of that AST.

[voxpelli]

Sorry for the spam here, but I did some more searching and found this module by @noahsug which seems to do the trick: https://www.npmjs.com/package/json5-writer

It didn't like null values, but apart from that, inserting the raw text of the source JSON and the parsed target JSON in the .write() seems to pretty much give the desired outcome in .toSource()

Should be possible to make use of without that many adaptions either.

[rarkins]

@rarkins would adding the prettierrc temporarily in the clone, without actually committing it, work?

@TWiStErRob unfortunately not a good idea to do that by default either, because most repos don't use prettier styling (especially if not from the JS ecosystem). While we could try to detect if a repo's config looks like it's prettier-formatted, I think it's reasonable for Renovate to look for any prettier config file instead.

[rarkins]

@voxpelli I like the idea of that library, but it's got some major challenges before we could adopt it:

• Low adoption
• Not maintained
• 0.x version
• Requires any changes to the AST to be done via writer.write() whereas we have written our migration to work on regular JS objects. We'd either need to change how we do migrations or add some type of mediation layer where we diff the final object and then apply changes to the writer

[voxpelli]

@rarkins My belief is that you can give the full modified JS object to write()

But yeah, would probably need to be forked and self-published unless the original author shows up.

[rarkins]

Adapting their example in this repo:

const fs = require('fs');
const json5Writer = require('json5-writer');

const config = fs.readFileSync('renovate.json', 'utf-8');
const writer = json5Writer.load(config);
writer.write({
  'eat honey': { cooldown: 3 },
  speak: { cooldown: 2 },
  bear: { actions: ['eat honey', 'speak'] },
});
fs.writeFileSync('renovate.json', writer.toSource(), 'utf-8');

It resulted in the renovate.json being fully wiped and only the new data remaining. If simply reading and writing it (i.e. dropping the writer.write() command then it's preserved. I wonder if the library is already broken somehow.

[voxpelli]

.write(value)

Updates the JSON / JSON5 string with the new value. Any field or property that doesn't exist in value is removed.

To keep an existing value, use undefined

@rarkins Either providing values for all keys, or values for all updates keys and setting the rest as undefined, would work

[rarkins]

So is their example in the README flawed? Why read in a file if you'll just blindly override it.

[voxpelli]

So is their example in the README flawed? Why read in a file if you'll just blindly override it.

If you look at the diff after the example code you can see that their config.json5 contains two of those fields.

But yes, one can argue that the example could be better and likewise the API, but it could be a good start 😌

I still think the ideal solution would be a JSON Patch library built on top of a white-space preserving AST they can then be serialized, but I also think that could be way out of scope for you to build here and that module was the closest my googling skills could find.

[reitzig]

FWIW: I for one am far less concerned with unwanted but easily remedied formatting changes (assuming my project is set up with linters and formatters to begin with wink 😉) than I am with my devs getting config migration proposals.

[rarkins]

Let's add this in the next major (v38)

[HonkingGoose]

@rarkins earlier you said you wanted issue #19783 done before making config migration the default:

I think we should first try to get more use of this feature by prompting people in the dependency dashboard that to open the migration PR "on demand": #19783

I guess #19783 is not blocking this issue anymore, because you removed the status:blocked label on this issue?

Just checking if we're really good to go to make config migration the default behavior soon. 😄

[rarkins]

Yes, I think we can

[gberche-orange]

I also notice that Config migration PRs removes any existing Json 5 comments in the existing config file, see https://github.com/orange-cloudfoundry/k3s-packages-boshrelease/pull/87/files#diff-31b1aa2b93afe11792d84c6cb5057a064b5a3294fa0caab92d7570ea604f759f

This therefore requires manual merge of this change to restore json5 comments

[HonkingGoose]

To summarize, I think we want to work on these issues before we start on the main issue?

• On-demand config migration PRs using Dashboard #19783
• Show user if config migration is needed for their own config or for an extends #21456
• Figure out how to preserve JSON5 comments during config migration???

I see users opening discussions and referring to old names (config:base for example):

• Weird extra merge request created #27792 (reply in thread)

It would be easier for us if Renovate users always used up-to-date terms and presets. Also it's easier for users to debug themselves when config and docs match.

I'll let the maintainers decide what we need to do, and when.

[rarkins]

Marking this as blocked as should enable opt-in via dashboard first