renovate icon indicating copy to clipboard operation
renovate copied to clipboard

fix(config-migration): closed migration prs are being ignored

Open Gabriel-Ladzaretti opened this issue 2 years ago • 14 comments

Changes

Currently a new migration PR would be created regardless of a closed one.

Now if we detect a closed migration pr, an Ignore Comment is added to the PR and we won't creating a new one.

Context

closes #16774

Documentation (please check one with an [x])

  • [ ] I have updated the documentation, or
  • [x] 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
  • [x] Both unit tests + ran on a real repository

Gabriel-Ladzaretti avatar Jul 25 '22 21:07 Gabriel-Ladzaretti

https://github.com/ladzaretti/feat-config-migration/pull/2

Gabriel-Ladzaretti avatar Jul 25 '22 21:07 Gabriel-Ladzaretti

i think a config migration PR should be immortal.

@rarkins WDYT?

viceice avatar Jul 26 '22 03:07 viceice

i think a config migration PR should be immortal.

@rarkins WDYT?

I think it's ok for us to have "close to ignore". Presumably that's what any user would intend if they close it.

Important though that a merged migration PR does not block a new one

rarkins avatar Jul 26 '22 03:07 rarkins

but wasn't the plan to remove old migration code from renovate when all repos are migrated some time in future?

if we allow to ignore those migrations, we will break them when removing the migration code.

that's why I think the migration PR should be immortal.

viceice avatar Jul 26 '22 04:07 viceice

but wasn't the plan to remove old migration code from renovate when all repos are migrated some time in future?

Yes, although who knows how achievable that will be. We might just end up removing selective migrations if for example we want to reuse an old config option name, while leaving the rest.

if we allow to ignore those migrations, we will break them when removing the migration code.

that's why I think the migration PR should be immortal.

Yes, but that's their choice. It reminds me a little about people who insist on ignoring Windows updates requests forever. Windows keeps popping up requests and those people get angrier.

Maybe we just let them close it but add a comment which makes it clear that their config may break in future, in which case they'll get a config validation issue instead.

rarkins avatar Jul 26 '22 04:07 rarkins

They can disable config migrations already (when it's enabled by default some time in future), that would block new PR too 🤷‍♂️

viceice avatar Jul 26 '22 04:07 viceice

They can disable config migrations already (when it's enabled by default some time in future), that would block new PR too 🤷‍♂️

I had thought the same, and for example same about dependency dashboard issues. But then the other day one user was practically outraged that when they closed the dashboard issue it came back again. So now I lean towards letting the user to be the boss, even if we think it's unwise or unreasonable.

One day we'll turn on migrations by default. There's definitely going to be some people who think "no thanks" and close the migration PR. The question is, do we or they gain anything by deciding to ignore that signal and reopening the PR until they either get so annoyed they uninstall, or figure out how to disable using config?

rarkins avatar Jul 26 '22 04:07 rarkins

Add a description and a link to the PR body how to disable?

viceice avatar Jul 26 '22 05:07 viceice

Add a description and a link to the PR body how to disable?

🔕 Ignore: Close this PR and you won't be reminded about this update again.

like we do for other PRs ?

Gabriel-Ladzaretti avatar Jul 26 '22 09:07 Gabriel-Ladzaretti

Needs to say something like "but one day your current config may no longer work"

rarkins avatar Jul 26 '22 11:07 rarkins

ignore message added -

https://github.com/ladzaretti/feat-config-migration/pull/5

Gabriel-Ladzaretti avatar Jul 26 '22 15:07 Gabriel-Ladzaretti

We have concluded that we will allow users to close to ignore

rarkins avatar Jul 29 '22 12:07 rarkins

fixed error due to merging main. can we try merging this again 😄?

Gabriel-Ladzaretti avatar Aug 03 '22 08:08 Gabriel-Ladzaretti

anything else to do here 😀?

Gabriel-Ladzaretti avatar Aug 07 '22 07:08 Gabriel-Ladzaretti

:tada: This PR is included in version 32.169.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

renovate-release avatar Aug 21 '22 05:08 renovate-release