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

GitHub Convention #588

Merged
merged 1 commit into from
Dec 21, 2024
Merged

GitHub Convention #588

merged 1 commit into from
Dec 21, 2024

Conversation

slawekjaranowski
Copy link
Member

No description provided.

GitHub Issue should be referenced in commit message by `#issue-number`.

GitHub Issue and Pull Request should have a label with type, like `bug`, `enhancement` and so on.
Pull Request without labels will be not categorized in Release Notes.
Copy link
Contributor

@elharo elharo Dec 7, 2024

Choose a reason for hiding this comment

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

Honestly this is overkill. Let's be cautious about making PRs more complex than they are already. I don't like any conventions for commit messages beyond say what the commit does. I've seen so many projects with all sorts of rules about how to format commit messages, and I've never once seen this be of any use at all. The cost vastly outweighs the benefits.

Copy link
Member Author

Choose a reason for hiding this comment

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

The big benefit is to categorize PR in release notes - we should list bug fixes, new features and so on ... grouped by category not in one plain list.

Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly this is overkill. Let's be cautious about making PRs more complex than they are already. I don't like any conventions for commit messages beyond say what the commit does. I've seen so many projects with all sorts of rules about how to format commit messages, and I've never once seen this be of any use at all. Thge cost vastly outweighs the benefits.

We kinda do the same in JIRA, we always assign a type to the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

This can be enforced if needed with something like:

name: Check PR Labels

on:
  pull_request:
    types: [opened, labeled, unlabeled, synchronize]

jobs:
  check-labels:
    runs-on: ubuntu-latest
    steps:
      - name: Check required labels
        uses: actions/github-script@v7
        with:
          script: |
            const requiredLabels = ['bug', 'enhancement', 'feature', 'dependencies', 'build', 'other'];
            const prLabels = context.payload.pull_request.labels.map(label => label.name);
            
            const hasRequiredLabel = requiredLabels.some(label => 
              prLabels.includes(label)
            );
            
            if (!hasRequiredLabel) {
              core.setFailed(
                'This PR must have one of the following labels: ' + 
                requiredLabels.join(', ')
              );
            } else {
              console.log('Label check passed! Found required label.');
            }

Copy link
Contributor

Choose a reason for hiding this comment

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

Enforcement is a major problem for non-comimtter contributors who cannot change labels. Let's not enforce. We can always clean uop labels later before generating release notes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Again, the issue is not committers. It's non-committer contributors. I'm not sure if this is something we can configure in the repo or not, but in other projects where I am not a committer I have noticed repeatedly that I am not able to apply or change labels for PRs I submit. Now maybe there's a way to give non-committer contributors the ability to edit labels on their own PRs and other projects just haven't bothered to do this. If so, fine. However, if this is a Github limitation, we need to plan for that.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is something what committer should care before merge, I don't see a problem for it.
If it is a problem, so what is your proposition ... how to categorize issues and PR and don't leave it for release manager.

Copy link
Member Author

Choose a reason for hiding this comment

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

I hope we can start working with such limitation and improve it in the future.

Copy link
Contributor

@ascheman ascheman Dec 10, 2024

Choose a reason for hiding this comment

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

You will need write access to the repository to edit labels, which usually is not the case for contributors, but only for committers. You could have an action which is bound to a custom slash command, like

  1. contributor wants a label added by adding a comment with /request-label feature
  2. committer approves the label request with /approve-label (which could also be auto-approved by rules)

That said, I doubt that many contributors would make use of it, since it is very uncommon and only very few people know about it. On the other hand it is not much work and probably worth a try. The question then is to educate contributors to learn and use the feature.

Personal opinion: Perhaps it is rather possible to have some AI (GH action) to evaluate PRs and propose labels or even auto-assign labels based on the contents.

Copy link
Member

Choose a reason for hiding this comment

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

Are contributors creating issues in Jira already able to add infos such components or labels?
frankly not sure what is the problem here.
rather the commit use the comment /request-label feature or the commiter add it before/after merging and that's it.

GitHub Issue and Pull Request should have a label with type, like `bug`, `enhancement` and so on.
Pull Request without labels will be not categorized in Release Notes.

Closed GitHub Issue and Pull Request should have milestone in which was resolved.
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't use milestones

Copy link
Member Author

Choose a reason for hiding this comment

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

But I think we should for project with GitHub Issues without Jira ... it is very useful for users.
It clear point to version which fix the historical issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly every project I've seen to date that tries to use Github milestones has de facto abandoned them. I don't quite understand why, but developers do seem to ignore milestones after the first few months,


Closed GitHub Issue and Pull Request should have milestone in which was resolved.

Pull Request title is used to generate Release Notes - should be similar or the same as merged commit.
Copy link
Contributor

Choose a reason for hiding this comment

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

No, titles need to be shorter

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes short but should provide a minimum information about change

When you read a release notes you want to understand what were going in release without reading each changes detail.


### Reviewers

We should invite persons to review for every change, even it is simply one, review can increase code quality.
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to make this mandatory and enforced by Github. No review, no commit. Don't do by convention what machines can enforce.


### Priority

For priority, we can use labels:
Copy link
Contributor

Choose a reason for hiding this comment

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

not necessary

Copy link
Member Author

Choose a reason for hiding this comment

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

we have such in Jira ... I think that can be useful to have a priority or importance for issues

Copy link
Contributor

Choose a reason for hiding this comment

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

Is that somehow recognized by GitHub ? can we order ?


For GitHub Issue and Pull Request we use labels, like:

- `bug`
Copy link
Contributor

Choose a reason for hiding this comment

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

Can non-committers assign labels? In many, perhaps all, repos they can't.

Copy link
Member Author

Choose a reason for hiding this comment

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

We should know what the kind issue, PR is.
Yes only committer can categorize it - but should not be a problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

In next steps we can work on issues templates ... but it should be a next discussion. By the way idea for issues template is very good.

@slawekjaranowski slawekjaranowski force-pushed the github-convention branch 3 times, most recently from 2f46b53 to 1127661 Compare December 8, 2024 16:48
The GitHub Issue can be created if you would like to discuss a proposition of change before start working on it.
In such cases developer mailing list can also be used instead of issue.

When you provide a Pull Request - creating separate issue is not necessary as you can describe your proposition in Pull Request.
Copy link
Contributor

Choose a reason for hiding this comment

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

is this really what we want? It sounds diffeerent than what's been proposed before.

Copy link
Member Author

Choose a reason for hiding this comment

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

As @olamy proposition, I agree that we don't need a issue for each PR, PR can describe a problem.

@elharo
Copy link
Contributor

elharo commented Dec 12, 2024 via email

@slawekjaranowski slawekjaranowski marked this pull request as ready for review December 15, 2024 11:24
@slawekjaranowski
Copy link
Member Author

I hope as at beginning it will be ok.
We still can improve it, make some of action automatically and so.

Copy link
Contributor

@gnodet gnodet left a comment

Choose a reason for hiding this comment

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

We need to use GH actions to automate as much as possible to streamline the process for contributors/committers/release managers.

@slawekjaranowski slawekjaranowski merged commit d23499b into master Dec 21, 2024
3 checks passed
@slawekjaranowski slawekjaranowski deleted the github-convention branch December 21, 2024 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants