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

fix: massage release notes links #17636

Closed
wants to merge 33 commits into from
Closed

fix: massage release notes links #17636

wants to merge 33 commits into from

Conversation

MaronHatoum
Copy link
Contributor

@MaronHatoum MaronHatoum commented Sep 5, 2022 •
edited

Changes

added new regex to massage What's Changed and New Contributors in release notes.
before the fix, these sections did not add a link to the user's profile, and the PRs links have been displayed incorrectly.

Context

closes #17404

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

@MaronHatoum
Copy link
Contributor Author

MaronHatoum commented Sep 5, 2022 •
edited

@MaronHatoum MaronHatoum marked this pull request as draft September 5, 2022 14:18
@MaronHatoum MaronHatoum changed the title fix: match all in collect link position function ignores colors #11 fix: release notes what's changed and New Contributors sections are not organized Sep 5, 2022
@MaronHatoum MaronHatoum changed the title fix: release notes what's changed and New Contributors sections are not organized fix(release notes): what's changed and New Contributors sections are not organized Sep 5, 2022
HonkingGoose
HonkingGoose suggested changes Sep 5, 2022
View reviewed changes
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 like this idea, but there's a problem. 😉

lib/modules/platform/github/massage-markdown-links.spec.ts Outdated Show resolved Hide resolved
viceice
viceice reviewed Sep 6, 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

@MaronHatoum
Merge branch 'main' of https://github.com/renovatebot/renovate into b…
2af9adf 
…ug/matchAll-in-collectLinkPosition-function-ignores-colors
@MaronHatoum
Merge branch 'main' of https://github.com/renovatebot/renovate into b…
5fd317b 
…ug/matchAll-in-collectLinkPosition-function-ignores-colors
@MaronHatoum
bug: update
e4458f0
@MaronHatoum
bug: update
2c880b3
@MaronHatoum MaronHatoum marked this pull request as ready for review September 6, 2022 10:07
@MaronHatoum MaronHatoum changed the title fix(release notes): what's changed and New Contributors sections are not organized fix: massage release notes links Sep 7, 2022
@MaronHatoum
Copy link
Contributor Author

Why do we need to arrange release notes? I don't understand what the issue is about

added more description.

@MaronHatoum MaronHatoum marked this pull request as ready for review September 7, 2022 08:47
@rarkins
Copy link
Collaborator

rarkins commented Sep 7, 2022

It looks like your PRs have been spamming pnpm, Eg pnpm/pnpm#5071

Has this been fixed?

@MaronHatoum
Copy link
Contributor Author

It looks like your PRs have been spamming pnpm, Eg pnpm/pnpm#5071

Has this been fixed?

yes

@rarkins
Copy link
Collaborator

rarkins commented Sep 12, 2022

I've labeled this PR as large impact, because if there's any mistake or misunderstanding here with this parsing then it could result in mass backlink spam. I'm not sure of the best way to monitor for this problem in production though because I'm not aware of any github.com search we can do to find our own backlinks. i.e. in a worst case scenario we could be backlink spamming for hours or days before we annoy someone enough to report it.

@MaronHatoum
Copy link
Contributor Author

MaronHatoum commented Sep 18, 2022 •
edited

I have checked the fix on a real repository.
if you take a look at this PR(after the fix) which has been created 11 days ago MaronHatoum/16921#49
and then you check the URLs in release notes you can see that the last "ping/spam" was 13 days ago which means that the fix I did here stopped spamming related PRs/discussions/issues that were mentioned in PR release notes.

Also, I added new unit tests to make sure that the URLs in release notes use togithub domain

@MaronHatoum
Merge branch 'main' of https://github.com/renovatebot/renovate into b…
e1d4f67 
…ug/matchAll-in-collectLinkPosition-function-ignores-colors
viceice
viceice reviewed Oct 12, 2022
View reviewed changes
lib/modules/platform/github/massage-markdown-links.ts Outdated Show resolved Hide resolved
lib/modules/platform/github/massage-markdown-links.spec.ts
${'https://github.com/foo/bar/discussions/1#comment-123'} | ${'[https://github.com/foo/bar/discussions/1#comment-123](https://togithub.com/foo/bar/discussions/1#comment-123)'}
${'https://github.com/foo/bar/issues/1#comment-123'} | ${'[https://github.com/foo/bar/issues/1#comment-123](https://togithub.com/foo/bar/issues/1#comment-123)'}
${'https://github.com/foo/bar/pull/1#comment-123'} | ${'[https://github.com/foo/bar/pull/1#comment-123](https://togithub.com/foo/bar/pull/1#comment-123)'}
${'github.com/foo/bar/discussions/1'} | ${'[foo/bar#1](togithub.com/foo/bar/discussions/1)'}
Copy link
Member

Choose a reason for hiding this comment

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

do those short links also work for discussions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

the answer is yes, but i will close this PR too and refactor it

Copy link
Collaborator

Choose a reason for hiding this comment

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

#17819 is doing this refactor, this PR is an aftermath for just one case

@PhilipAbed
Copy link
Collaborator

closing this as maroon is no longer working on it, opened #18944 instead

@PhilipAbed PhilipAbed closed this Nov 16, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@viceice viceice viceice left review comments

@Gabriel-Ladzaretti Gabriel-Ladzaretti Awaiting requested review from Gabriel-Ladzaretti

@nabeelsaabna nabeelsaabna Awaiting requested review from nabeelsaabna

@PhilipAbed PhilipAbed Awaiting requested review from PhilipAbed

@HonkingGoose HonkingGoose Awaiting requested review from HonkingGoose

@rarkins rarkins Awaiting requested review from rarkins

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

Successfully merging this pull request may close these issues.

matchAll in collectLinkPosition function ignores colors number when getting startOffset
5 participants
@MaronHatoum @PhilipAbed @rarkins @viceice @HonkingGoose