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

[Feature]: (Re)Introduce changelog-related labels and require them for PRs #4799

Closed
3 of 4 tasks
yurishkuro opened this issue Oct 4, 2023 · 15 comments · Fixed by #4850, #4854 or jaegertracing/jaeger-ui#1897
Closed
3 of 4 tasks
Labels
good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement

Comments

@yurishkuro
Copy link
Member

yurishkuro commented Oct 4, 2023

We already tried this in the past, but there was no enforcement and it didn't take.

@yurishkuro yurishkuro added help wanted Features that maintainers are willing to accept but do not have cycles to implement good first issue Good for beginners labels Oct 4, 2023
@sanyamjain036
Copy link

I'm a beginner to github actions but I want to work on this issue. I am learning about github actions online. May you please guide how to learn and solve this issue?

@Suraj-kumar00
Copy link

Hi @yurishkuro, How are you doing?
I saw no one is working on this issue so can you please assign it to me so that i can work on this issue?

@yurishkuro
Copy link
Member Author

### Assigning Issues
We do not assign issues to contributors. It is almost never the case that multiple
people jump on the same issue, and practice showed that occasionally people who ask
for an issue to be assigned to them later have a change in priorities and are unable
to find time to finish it, which leaves the issue in limbo.
So if you have a desire to work on an issue, feel free to mention it in the comment and just submit a PR.

@Suraj-kumar00
Copy link

### Assigning Issues
We do not assign issues to contributors. It is almost never the case that multiple
people jump on the same issue, and practice showed that occasionally people who ask
for an issue to be assigned to them later have a change in priorities and are unable
to find time to finish it, which leaves the issue in limbo.
So if you have a desire to work on an issue, feel free to mention it in the comment and just submit a PR.

Yeah thanks, Now I'm doing this!

@kanha-gupta
Copy link

Hey @Suraj-kumar00 are you currently working on this ?

@devidasjadhav
Copy link

@yurishkuro Can you please explain " to validate that all PRs have changelog labels" means ?
a44a245

As this was the only PR with changelog.
So I am definitely missing something here.
Please explain.

@jkowall
Copy link
Contributor

jkowall commented Oct 6, 2023

@devidasjadhav We are trying to ensure that each PR has a changelog label, which would allow us to generate release notes more easily and in an automated way. Currently, the release process generates the list, but requires some manual processing of the ones which are not tagged.

@jkowall
Copy link
Contributor

jkowall commented Oct 6, 2023

A couple pointer is looking at this existing action: https://github.com/marketplace/actions/pr-labels and this repo : https://github.com/actions/labeler which is using these actions too.

The current release notes script is located here too: https://github.com/jaegertracing/jaeger/blob/main/scripts/release-notes.py

@Suraj-kumar00
Copy link

Hey @Suraj-kumar00 are you currently working on this ?

Yes.

akagami-harsh added a commit to akagami-harsh/jaeger that referenced this issue Oct 16, 2023
yurishkuro added a commit that referenced this issue Oct 16, 2023
#4847)

## Which problem is this PR solving?
- part of: #4799
- [x] Add a GitHub Action that will verify that a PR has a specific
categorization label before it can be merged. We already have a bunch of
those labels changelog:*** labels

## Description of the changes
- This workflow would first check if the PR is created by @dependabot or
not. If no, it would proceed to check for the labels on that pull
request. For example:
- A new PR is created. The Label Check workflow would fail
- A maintainer adds some label `bug` on the PR, workflow would again
run, but would still fail since the label `bug` doesn't start with
`changelog:`
- The maintainer now adds some label `changelog:**`, this time, Label
Check CI would pass.

## How was this change tested?
- Without any label, failed GH Action:
https://github.com/anshgoyalevil/jaeger/actions/runs/6531596646/attempts/2
- With the Label `changelog: feature`: Passing GH Action:
https://github.com/anshgoyalevil/jaeger/actions/runs/6531596646/attempts/3
- With the label `bug` and not `changelog: feature`:
https://github.com/anshgoyalevil/jaeger/actions/runs/6531596646/attempts/4
- With three labels `bug`, `documentation`, and `changelog: feature`:
https://github.com/anshgoyalevil/jaeger/actions/runs/6531596646

## How to test this PR?
- Play with the labels by applying or removing them and see the Verify
Label action fail/pass.

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [x] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`.

---------

Signed-off-by: Ansh Goyal <[email protected]>
Co-authored-by: Yuri Shkuro <[email protected]>
yurishkuro pushed a commit that referenced this issue Oct 17, 2023
… labels (#4850)

## Which problem is this PR solving?
- fixes: #4799

## Description of the changes
- The modifications in release-notes.py would now make sure that the
final results printed to the console are in a categorized manner which
could be just copy-pasted to release-notes.
- The `categories` object could be modified to accommodate more
categories along with some meaningful titles like `Changes related to
CI: changelog:ci`

## How was this change tested?
- Locally, on the jaegertracing/jaeger repo.

![image](https://github.com/jaegertracing/jaeger/assets/94157520/ba1cb4a8-dcf9-4b25-bad4-ba812d7772fa)


## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [x] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

---------

Signed-off-by: Ansh Goyal <[email protected]>
@yurishkuro yurishkuro reopened this Oct 17, 2023
@yurishkuro
Copy link
Member Author

I just ran the script and it shows all dependabot PRs under Uncategorized, instead of skipping them.

BTW, instead of building special logic for dependabot, we can change its config to add changelog:skip label (now it only sets dependencies label).

cc @anshgoyalevil

yurishkuro added a commit that referenced this issue Oct 17, 2023
## Which problem is this PR solving?
- Resolves #4799

## Description of the changes
- Configure dependabot to use `changelog:dependencies` label
- Skip that label in release notes
- Print progress when loading PRs

## How was this change tested?
- local run

## Checklist
- [ ] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [ ] I have signed all commits
- [ ] I have added unit tests for the new functionality
- [ ] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

---------

Signed-off-by: Yuri Shkuro <[email protected]>
@anshgoyalevil
Copy link
Member

@yurishkuro Did you used the --skip-dependabot flag?

We can change the dependabot labelling settings but that would defeat the purpose of --skip-dependabot flag

@yurishkuro
Copy link
Member Author

Ha, I didn't know we had that flag. Well if anything it should be --include-dependabot, so that it's off by default, but we never really include those in the changelog anyway.

@anshgoyalevil
Copy link
Member

Yes, then a better workaround here would be to rename the --skip-dependabot to --include-dependabot arg, and use it accordingly, but since #4854 solves this purpose anyhow, do we need to proceed with this arg renaming thing?

@yurishkuro
Copy link
Member Author

No, I am good with the current state

@yurishkuro
Copy link
Member Author

We need to use these new scripts in jaeger-ui repo as well.

@yurishkuro yurishkuro reopened this Oct 20, 2023
yurishkuro pushed a commit to jaegertracing/jaeger-ui that referenced this issue Oct 21, 2023
#1897)

## Which problem is this PR solving?
- resolves jaegertracing/jaeger#4799

## Description of the changes
- [x] Add a GitHub Action that will verify that a PR has a specific
categorization label before it can be merged. We already have a bunch of
those labels changelog:*** labels
- [x] Configure dependabot to use `changelog:dependencies` label
- Since we already use the release script from `jaeger` repo, everything
else is setup 🚀
- One task is left, that is to create changelog labels inside this
`jaeger-ui` repository. //cc: @yurishkuro


https://github.com/jaegertracing/jaeger-ui/blob/1909398b498b8f4536c6a58809c2fe1eb88b062a/Makefile#L2-L5

## How was this change tested?
- 

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [x] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

---------

Signed-off-by: Ansh Goyal <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement
Projects
None yet
7 participants