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

ci: verify changes to Maintainers.yaml made by the bot #750

Merged
merged 45 commits into from
Aug 9, 2023

Conversation

14Richa
Copy link
Contributor

@14Richa 14Richa commented Jun 15, 2023

Description

This pull request includes a workflow triggered when changes are made to the Maintainers.yaml file. It checks for new maintainer objects, removes maintainer objects, and modifies GitHub usernames and repository lists.

This workflow has 3 jobs:

  • verify-changes: This job compares changes in the MAINTAINERS.yaml file between the main branch and the pull request, identifies removed TSC members by the asyncapi-bot, and checks for critical changes made by humans.

  • handle-human-made-critical-changes: If there are critical changes detected in the MAINTAINERS.yaml file, this job posts a comment on the pull request with the error messages and closes the pull request.

  • bot-removal-notification: This job notifies maintainers when the asyncapi-bot removes a TSCmember from the MAINTAINERS.yaml file by posting a comment on the pull request.

I have tested the workflow in my testRepo you can find the logs : https://github.com/14Richa/testRepo/actions/runs/5750090267

Note that the Maintainers.yaml file is expected to be in a separate #720 that has not yet been merged.

Related issue(s)
#210

@14Richa 14Richa marked this pull request as ready for review June 16, 2023 05:40
Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

I see you prefer bash over js 😄

  • why do we need two POST calls? call to reviews endpoint should be enough, no?
  • it is not such simple validation of the actor. We discussed that there is a case for human edit, when people want to update some information about them, like linkedin, or company affiliation for example. So validation should block PR from merging only if human creates a PR that adds or remove new maintainer object imho

.github/workflows/validate-maintainers.yml Outdated Show resolved Hide resolved
.github/workflows/validate-maintainers.yml Outdated Show resolved Hide resolved
.github/workflows/validate-maintainers.yml Outdated Show resolved Hide resolved
@14Richa 14Richa changed the title ci: validate changes to Maintainers.yaml made by the bot ci: verify changes to Maintainers.yaml made by the bot Jun 27, 2023
.github/workflows/verify-maintainers.yaml Outdated Show resolved Hide resolved
.github/workflows/verify-maintainers.yaml Outdated Show resolved Hide resolved
.github/scripts/verify-maintainers-changes.js Outdated Show resolved Hide resolved
Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

I see PR title verify changes to Maintainers.yaml made by the bot but no code that verifies that changes are made by bot. Can't review as not sure what is the context an cannot understand why bot cannot remove maintainer and that PR is closed

.github/workflows/verify-maintainers.yaml Outdated Show resolved Hide resolved
@14Richa
Copy link
Contributor Author

14Richa commented Jul 3, 2023

I see PR title verify changes to Maintainers.yaml made by the bot but no code that verifies that changes are made by bot.

Thank you @derberg for pointing this out. It was an oversight, and I've now added the check to verify if changes are made by the bot.

.github/workflows/verify-maintainers.yaml Outdated Show resolved Hide resolved
.github/workflows/verify-maintainers.yaml Outdated Show resolved Hide resolved
.github/workflows/verify-maintainers.yaml Outdated Show resolved Hide resolved
.github/workflows/verify-maintainers.yaml Outdated Show resolved Hide resolved
.github/workflows/verify-maintainers.yaml Outdated Show resolved Hide resolved
.github/workflows/verify-maintainers.yaml Outdated Show resolved Hide resolved
.github/workflows/verify-maintainers.yaml Outdated Show resolved Hide resolved
.github/workflows/verify-maintainers.yaml Outdated Show resolved Hide resolved
.github/workflows/verify-maintainers.yaml Outdated Show resolved Hide resolved
.github/workflows/verify-maintainers.yaml Outdated Show resolved Hide resolved
14Richa and others added 5 commits July 4, 2023 19:47
Co-authored-by: Lukasz Gornicki <lpgornicki@gmail.com>
Co-authored-by: Lukasz Gornicki <lpgornicki@gmail.com>
Co-authored-by: Lukasz Gornicki <lpgornicki@gmail.com>
Co-authored-by: Lukasz Gornicki <lpgornicki@gmail.com>
@14Richa
Copy link
Contributor Author

14Richa commented Jul 11, 2023

@14Richa left some comments, please also update description with latest tests as it is pointing to old logs I think

Thanks for the review. I have updated the description with recent logs.

Copy link
Member

@KhudaDad414 KhudaDad414 left a comment

Choose a reason for hiding this comment

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

one small thing, other than that, looks great!

.github/workflows/verify-maintainers.yaml Outdated Show resolved Hide resolved
Co-authored-by: Khuda Dad Nomani <32505158+KhudaDad414@users.noreply.github.com>
@14Richa
Copy link
Contributor Author

14Richa commented Jul 11, 2023

one small thing, other than that, looks great!

Thanks @KhudaDad414, I have made the requested changes.

Copy link
Member

@KhudaDad414 KhudaDad414 left a comment

Choose a reason for hiding this comment

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

LGTM!

@derberg
Copy link
Member

derberg commented Jul 11, 2023

shouldn't this fail 14Richa/testRepo#417 ?

@14Richa
Copy link
Contributor Author

14Richa commented Jul 11, 2023

shouldn't this fail 14Richa/testRepo#417 ?

Yep @derberg. I have made minor fixes. Now it's working fine. You can take a look here : 14Richa/testRepo#420

14Richa and others added 3 commits July 12, 2023 21:30
Co-authored-by: Lukasz Gornicki <lpgornicki@gmail.com>
derberg
derberg previously approved these changes Jul 13, 2023
Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

👏🏼

@derberg
Copy link
Member

derberg commented Jul 13, 2023

/dnm

making sure it won't get merged by any automation to early

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

lgtm

@derberg
Copy link
Member

derberg commented Aug 9, 2023

/rtm

@asyncapi-bot asyncapi-bot merged commit 23644a4 into asyncapi:master Aug 9, 2023
9 checks passed
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.

4 participants