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

refactor(migrations): migrate depTypes #16421

Merged

Conversation

RahulGautamSingh
Copy link
Collaborator

@RahulGautamSingh RahulGautamSingh commented Jul 5, 2022

Changes

  • migrate depTypes using AbstractMigration

Context

Documentation (please check one with an [x])

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

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

@RahulGautamSingh RahulGautamSingh marked this pull request as draft July 5, 2022 00:54
@RahulGautamSingh

This comment was marked as outdated.

@RahulGautamSingh

This comment was marked as outdated.

@RahulGautamSingh RahulGautamSingh requested review from rarkins, pret-a-porter and viceice and removed request for pret-a-porter July 8, 2022 05:52
@RahulGautamSingh RahulGautamSingh changed the title refactor(migrations): migrate depTypes[WIP] refactor(migrations): migrate depTypes Jul 8, 2022
@RahulGautamSingh RahulGautamSingh marked this pull request as ready for review July 8, 2022 13:16
Copy link
Collaborator

@rarkins rarkins left a comment

Choose a reason for hiding this comment

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

It looks like some package rules changed order in the tests. Is this correct/expected?

@RahulGautamSingh
Copy link
Collaborator Author

RahulGautamSingh commented Jul 10, 2022

It looks like some package rules changed order in the tests. Is this correct/expected?

Expected but not correct. There were 2 cases to handle:

  1. depTypes option (handled here)
  2. Options such as devDependencies, peerDependencies, and others were not handled because they caused problems with imports of other functions. I attempted to make it work by handling only major parts of their entire logic in the custom class, as I had done with other options that used migrateConfig or mergeChildConfig in their migration-logic, but it did not work in this case. As a result, these options are migrated the old-fashioned way and they are added to packageRules after all other options have been migrated.

Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

can we somehow avoid the snapshot changes?

lib/config/migrations/custom/dep-types-migration.ts Outdated Show resolved Hide resolved
@rarkins
Copy link
Collaborator

rarkins commented Jul 10, 2022

If you say it's not correct then does that mean you plan to fix it?

@RahulGautamSingh
Copy link
Collaborator Author

RahulGautamSingh commented Jul 10, 2022

can we somehow avoid the snapshot changes?

I can't think of any way to Using PackageRule and PackageRuleInputConfig inpace of any

@RahulGautamSingh
Copy link
Collaborator Author

RahulGautamSingh commented Jul 10, 2022

If you say it's not correct then does that mean you plan to fix it?

Yes, I want to and tried some approaches but none worked.
I am going to look into the issue which causes the imports to fail my guess is a race-condition so far.
We can convert it to draft and merge when the issue is fixed or that can be done in a future PR.

I have pushed new changes which are a way of handling it.

Copy link
Collaborator

@rarkins rarkins left a comment

Choose a reason for hiding this comment

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

Can these migrations be combined into a single one, or best to keep separate? They seem quite similar

lib/config/migration.ts Outdated Show resolved Hide resolved
@RahulGautamSingh
Copy link
Collaborator Author

RahulGautamSingh commented Jul 12, 2022

Can these migrations be combined into a single one, or best to keep separate? They seem quite similar

I tried doing so but failed. The reason was that I tried to migrate them in the same-class as depTypes by adding an extra for-loop. It didn't work cause they didn't get migrated when depTypes option wasn't present in config.

So I had two ideas in mind:

  1. Add that extra for-loop in the migration-class of an option which is always present (kinda hacky)
  2. Have separate classes for them (not happy, cause redundant)
    I went with the second option.

IMO Best to keep separate. Even though they are related we treat them each as different options in config.

lib/config/migration.ts Outdated Show resolved Hide resolved
lib/config/migrations/custom/dep-types-migration.ts Outdated Show resolved Hide resolved
lib/config/migrations/custom/dependencies-migration.ts Outdated Show resolved Hide resolved
lib/config/migrations/custom/dev-dependencies-migration.ts Outdated Show resolved Hide resolved
lib/config/migration.ts Outdated Show resolved Hide resolved
RahulGautamSingh and others added 2 commits July 19, 2022 11:50
Co-authored-by: Michael Kriese <michael.kriese@visualon.de>
@viceice viceice enabled auto-merge (squash) July 21, 2022 04:55
@viceice viceice merged commit 5b74dad into renovatebot:main Jul 21, 2022
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 32.121.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 21, 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.

None yet

4 participants