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

fix(ci): improve attribution of co-authors #4104

Merged

Conversation

zvolin
Copy link
Contributor

@zvolin zvolin commented Jun 23, 2023

Description

Fix the Co-authored-by inclusion in commit messages so that co-authorship is properly expressed. Additionally, filter merge commits before unique authors. Previously, we would not attribute an author if their first commit in a PR was a merge commit. Finally, we remove superfluous newlines between the Co-authored-by lines.

Notes & open questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@thomaseizinger thomaseizinger changed the title chore(ci): fix Co-authored-by case to attribute contribs fix(ci): use correct casing for Co-authored-by Jun 23, 2023
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Thank you and sorry again that this didn't work properly.

As mentioned in the other PR I am in contact with GitHub support about this.

@zvolin
Copy link
Contributor Author

zvolin commented Jun 23, 2023

I'm also wondering if it will work with an empty lines between multiple co-authored-by, here github docs mentions it should on the very end, and we got something like this
85a846a

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Jun 23, 2023

I'm also wondering if it will work with an empty lines between multiple co-authored-by, here github docs mentions it should on the very end, and we got something like this
85a846a

I was bothered with the empty lines too, although so far I considered it to be purely cosmetic.

I did open a discussion upstream already but hadn't had the time yet to try it out: Mergifyio/mergify#5091

I used https://j2live.ttl255.com/ in the past to test out the template rendering without having to iterate by merging PRs. The commits data structure is documented here: https://docs.mergify.com/configuration/#commits

.github/mergify.yml Outdated Show resolved Hide resolved
@thomaseizinger thomaseizinger changed the title fix(ci): use correct casing for Co-authored-by fix(ci): improve attribution of co-authors Jun 23, 2023
@thomaseizinger
Copy link
Contributor

Argh, I forgot that mergify doesn't support updating variables within a loop.

@thomaseizinger
Copy link
Contributor

Yey! I think I managed to get this to work!

@thomaseizinger thomaseizinger merged commit 41076f6 into libp2p:master Jun 27, 2023
thomaseizinger added a commit that referenced this pull request Jun 27, 2023
Unfortunately, the automated contribution message does not work properly. To evaluate this correctly, we need to first filter out all merge commits. To do this, we need to check the length of the `parents` property. I did not find a way to do this with jinja2 filters. Which would allow us to do something like `rejectattr`.

In the code snippet removed in this PR, I attempted to create a property to filter the merge commits on. This also doesn't work because the underlying object exposed by mergify is not a dictionary but a class and thus does not have the `update` function.

Mergify is shipping an additional property next month: Mergifyio/mergify#4636 (reply in thread). Thus, I am replacing this automated mechanism with a manual section that we can add to the PR until mergify ships the additional property that we can utilize to implement this in a clean way.

Related: #4130.
Related: #4104.

Pull-Request: #4131.
@zvolin zvolin deleted the chore/fix-co-authored-by-messages branch June 28, 2023 11:27
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.

2 participants