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.

Sign up for GitHub

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

Jump to bottom

feat: branchPrefixOld #15591

Merged
merged 9 commits into from
May 28, 2022
Merged

feat: branchPrefixOld #15591

merged 9 commits into from
May 28, 2022

Conversation

Gabriel-Ladzaretti
Copy link
Collaborator

@Gabriel-Ladzaretti Gabriel-Ladzaretti commented May 15, 2022 •
edited
Loading

Changes

Ignore closed PRs with branchName = branchPrefixOld/update in addition to default branchPrefix/update ones.

Context

closes #15496

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

@Gabriel-Ladzaretti Gabriel-Ladzaretti force-pushed the 15496-feat-branchPrefixOld branch from 766df40 to 94d50e0 Compare May 15, 2022 16:46
@viceice
Copy link
Member

viceice commented May 16, 2022

Does thios close #15496? Then please add - closes before issue id to autoclose issue after merge

viceice
viceice reviewed May 16, 2022
View reviewed changes
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.

otherwise lgtm

lib/workers/repository/update/branch/check-existing.spec.ts Outdated Show resolved Hide resolved
lib/workers/repository/update/branch/check-existing.spec.ts Outdated Show resolved Hide resolved
HonkingGoose
HonkingGoose suggested changes May 16, 2022
View reviewed changes
docs/usage/configuration-options.md Outdated Show resolved Hide resolved
docs/usage/configuration-options.md Outdated Show resolved Hide resolved
docs/usage/configuration-options.md Outdated Show resolved Hide resolved
viceice
viceice reviewed May 16, 2022
View reviewed changes
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.

LGTM, waiting for docs approval

@Gabriel-Ladzaretti Gabriel-Ladzaretti requested review from viceice and HonkingGoose and removed request for viceice May 16, 2022 14:17
rarkins
rarkins reviewed May 16, 2022
View reviewed changes
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.

Have you considered if you can also use this for currently open PRs?

@Gabriel-Ladzaretti
Copy link
Collaborator Author

Have you considered if you can also use this for currently open PRs?

i have not, in this case we will end up with a duplicate pr.
https://github.com/ladzaretti/cfg-testing-json5/pulls

the obvious solution would be to check for an open pr with the oldprefix and if there is one, then we need not create a new one.

WDYT?

@rarkins
Copy link
Collaborator

rarkins commented May 17, 2022

the obvious solution would be to check for an open pr with the oldprefix and if there is one, then we need not create a new one.

Yes, that would be ideal. Can you see how complex any implementation would be?

@Gabriel-Ladzaretti
Copy link
Collaborator Author

Gabriel-Ladzaretti commented May 17, 2022 •
edited
Loading

Yes, that would be ideal. Can you see how complex any implementation would be?

pretty straight forward really, in branch/index we first check if config.branchName (with branch prefix) exists. if it does not exist, we will check for the same branch but with the old prefix, if this one exists we just need to update config.branchName to that and proceed as is.

i have this working and tested all scenarios i could think of, and it works as it should.

just need to add coverage thats it. all in all its pretty much the same logic we have already implemented in check-existing the only diff is that here we are searching for open prs thats it.

i might as well commit the changes instead, as its a shorter read 😂

@Gabriel-Ladzaretti
feat(repo/update): branchPrefix migration with branchPrefixOld
678029b 
  - add support for open prs with prefixOld
@viceice viceice requested a review from rarkins May 18, 2022 15:14
viceice
viceice reviewed May 18, 2022
View reviewed changes
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.

code LGTM

rarkins
rarkins requested changes May 28, 2022
View reviewed changes
docs/usage/configuration-options.md Outdated Show resolved Hide resolved
lib/config/options/index.ts Show resolved Hide resolved
lib/workers/repository/update/branch/check-existing.ts Show resolved Hide resolved
lib/workers/repository/update/branch/index.ts Show resolved Hide resolved
@rarkins rarkins changed the title feat(repo/update): branchPrefix migration with branchPrefixOld feat: branchPrefixOld May 28, 2022
@rarkins rarkins enabled auto-merge (squash) May 28, 2022 11:42
@rarkins rarkins merged commit 50d9ded into renovatebot:main May 28, 2022
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 32.68.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Gabriel-Ladzaretti Gabriel-Ladzaretti deleted the 15496-feat-branchPrefixOld branch June 26, 2022 07:22
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rarkins rarkins rarkins approved these changes

@viceice viceice viceice approved these changes

@HonkingGoose HonkingGoose Awaiting requested review from HonkingGoose

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

branchPrefixOld
5 participants
@Gabriel-Ladzaretti @viceice @rarkins @renovate-release @HonkingGoose