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

🐛 Deduplicating area in pr title in release notes #9186

Merged
merged 1 commit into from
Aug 28, 2023

Conversation

Dhairya-Arora01
Copy link
Contributor

@Dhairya-Arora01 Dhairya-Arora01 commented Aug 14, 2023

What this PR does / why we need it:Deduplicating prefix in release notes as stated in #9104 Release Notes section 3rd task

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes: #9190
Part of: #9104

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 14, 2023
@k8s-ci-robot k8s-ci-robot added do-not-merge/needs-area PR is missing an area label needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 14, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @Dhairya-Arora01. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Copy link
Contributor

@killianmuldoon killianmuldoon left a comment

Choose a reason for hiding this comment

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

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 14, 2023
@Dhairya-Arora01 Dhairya-Arora01 changed the title Deduplicating area in pr title in release notes 🐛 Deduplicating area in pr title in release notes Aug 14, 2023
@vincepri vincepri added the area/release Issues or PRs related to releasing label Aug 14, 2023
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/needs-area PR is missing an area label label Aug 14, 2023
@furkatgofurov7
Copy link
Member

@Dhairya-Arora01 hey, thanks for the PR.

Can you please create a separate issue where this PR tries to tackle it, that is because we agreed to create new issues for each individual sub-tasks since #9104 is an umbrella issue. Once you create a separate issue dedicated for this PR, you can update the PR description with:

Fixes: #<newly created issue>
Part of: #9104 

@Dhairya-Arora01
Copy link
Contributor Author

@Dhairya-Arora01 hey, thanks for the PR.

Can you please create a separate issue where this PR tries to tackle it, that is because we agreed to create new issues for each individual sub-tasks since #9104 is an umbrella issue. Once you create a separate issue dedicated for this PR, you can update the PR description with:

Fixes: #<newly created issue>
Part of: #9104 

Sure

Copy link
Contributor

@g-gaston g-gaston left a comment

Choose a reason for hiding this comment

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

thanks a lot!

hack/tools/release/notes.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 15, 2023
@g-gaston
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 16, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 579e4e67b7e89f98eba7daefbf3995add1523a8a

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 18, 2023
@k8s-ci-robot k8s-ci-robot requested a review from g-gaston August 18, 2023 02:59
Copy link
Member

@furkatgofurov7 furkatgofurov7 left a comment

Choose a reason for hiding this comment

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

Thanks @Dhairya-Arora01

/lgtm
/cc @sbueringer @killianmuldoon
@kubernetes-sigs/cluster-api-release-team

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 18, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: b9520910a3b756a48fc0a376ebb629a91db29f22

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 19, 2023
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 19, 2023
}{
{
name: "PR title with area",
title: "test/e2e: improve logging for a detected rollout",
Copy link
Contributor

Choose a reason for hiding this comment

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

this made me look at the implementation and I think it might a bit dangerous
The "search" we do is global, so if a PR title has the prefix in the middle of the string, we will "cut" from there. Instead, I think it would be better if we remove the area only when we find it at the start of the string.

example of problematic title:
Update API:Machine with new field
will be converted to
Machine with new field

Copy link
Contributor Author

@Dhairya-Arora01 Dhairya-Arora01 Aug 22, 2023

Choose a reason for hiding this comment

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

@g-gaston the area that we are removing is user friendly area

area, err = getAreaLabel(c.merge)

Example in our case test/e2e: but the user friendly area is e2e and it makes sense to remove whole test/e2e:

Also we are searching for area + ":" to remove , and i think it doesn't makes sense to have area: in between the PR title.
WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also we are searching for area + ":" to remove , and i think it doesn't makes sense to have area: in between the PR title.

I don't disagree but I don't want to make that restriction for PR titles. The idea here is to cleanup PR titles for the release notes when we can, not to impose more rules over PR titles. I would prefer to restrict the cleanup to just the beginning of the title. If that means we can't automatically cleanup some titles and we do it manually, it's an ok tradeoff.

Example in our case test/e2e: but the user friendly area is e2e and it makes sense to remove whole test/e2e:

I understand what your are saying about that PR specifically, but we can't really catch all the possible variations. What if the author starts the PR title with Test/E2E? We won't be able to catch that with the current logic, although it's still duplicated.

If what you want is to be able to catch composed prefixes with the area, why don't you use a regex? That way you can restrict it to start at the beginning of the title and maybe even make it case insensitive.
Something like (I haven't tested this btw, just a direction to explore)

`(?mi)^\S*/e2e:\s*`

@Dhairya-Arora01 Dhairya-Arora01 force-pushed the deDuplicate branch 2 times, most recently from eb41fc5 to 0cecd77 Compare August 25, 2023 03:24
@sbueringer
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 25, 2023
Copy link
Member

@furkatgofurov7 furkatgofurov7 left a comment

Choose a reason for hiding this comment

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

This looks good to me, but leaving it to others take another look after suggestions

/cc @g-gaston

/lgtm
/hold

@k8s-ci-robot k8s-ci-robot requested a review from g-gaston August 25, 2023 08:49
@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 25, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: b068bf696c6ad261923479cc0a486537fbd59f01

@g-gaston
Copy link
Contributor

/lgtm
/unhold

Thanks!

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 28, 2023
@k8s-ci-robot k8s-ci-robot merged commit 5289526 into kubernetes-sigs:main Aug 28, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.6 milestone Aug 28, 2023
@Dhairya-Arora01 Dhairya-Arora01 deleted the deDuplicate branch August 28, 2023 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/release Issues or PRs related to releasing cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deduplicate prefixes in release notes.
7 participants