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

feat!: Change Hook Config field from map to *HookConfig #3073

Merged
merged 4 commits into from Feb 17, 2024

Conversation

himazawa
Copy link
Contributor

@himazawa himazawa commented Feb 13, 2024

Fixes: #3072.

BREAKING CHANGE: Changes Hook.Config from map[string]interface{} to *HookConfig.

Copy link

google-cla bot commented Feb 13, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@himazawa
Copy link
Contributor Author

I would like to sign the CLA but there is no way to add the noreply.github.com email to the CLA account.

@himazawa himazawa changed the title chore!: change Hook Config field from map to *HookConfig Change Hook Config field from map to *HookConfig Feb 14, 2024
Copy link
Collaborator

@gmlewis gmlewis 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, @himazawa.

github/hooks_config.go Outdated Show resolved Hide resolved
@gmlewis
Copy link
Collaborator

gmlewis commented Feb 14, 2024

I would like to sign the CLA but there is no way to add the noreply.github.com email to the CLA account.

Did you read the instructions for email addresses here: https://github.com/google/go-github/pull/3073/checks?check_run_id=21540833172 ?

Copy link

codecov bot commented Feb 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2b8c7fa) 97.72% compared to head (ea29fb5) 92.86%.
Report is 8 commits behind head on master.

❗ Current head ea29fb5 differs from pull request most recent head bf1d28c. Consider uploading reports for the commit bf1d28c to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3073      +/-   ##
==========================================
- Coverage   97.72%   92.86%   -4.86%     
==========================================
  Files         153      170      +17     
  Lines       13390    11400    -1990     
==========================================
- Hits        13085    10587    -2498     
- Misses        215      723     +508     
  Partials       90       90              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@himazawa
Copy link
Contributor Author

himazawa commented Feb 14, 2024

Did you read the instructions for email addresses here: https://github.com/google/go-github/pull/3073/checks?check_run_id=21540833172 ?

@gmlewis yes I did, there is no way from the CLA panel to change the email, other than adding it from gmail.
The issue is that gmail considers 73994521+himazawa​@users.noreply.github.com an invalid email.

@gmlewis
Copy link
Collaborator

gmlewis commented Feb 14, 2024

Did you read the instructions for email addresses here: https://github.com/google/go-github/pull/3073/checks?check_run_id=21540833172 ?

@gmlewis yes I did, there is no way from the CLA panel to change the email, other than adding it from gmail. The issue is that gmail considers 73994521+himazawa​@users.noreply.github.com an invalid email.

That is a throw-away email address that you will never use. Mine is 6143982+gmlewis@users.noreply.github.com and it works fine and I never use it, so I'm not sure I understand what the problem is. I registered my real email address with the Google CLA, signed it, and then added the fake email to GitHub (in addition to my real one) and use the fake email in my ~/.gitconfig and everything works fine.

@gmlewis
Copy link
Collaborator

gmlewis commented Feb 14, 2024

Well, looks like you got it resolved whatever the problem was.

@gmlewis gmlewis added NeedsReview PR is awaiting a review before merging. Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). labels Feb 14, 2024
@gmlewis
Copy link
Collaborator

gmlewis commented Feb 14, 2024

Please run step 4 of CONTRIBUTING.md and then push (not force-push) the changes to this PR.

@himazawa
Copy link
Contributor Author

Done

Copy link
Collaborator

@gmlewis gmlewis 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, @himazawa !
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

Copy link
Contributor

@valbeat valbeat left a comment

Choose a reason for hiding this comment

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

LGTM!

@gmlewis
Copy link
Collaborator

gmlewis commented Feb 17, 2024

Thank you, @valbeat !
Merging.

@gmlewis gmlewis changed the title Change Hook Config field from map to *HookConfig feat!: Change Hook Config field from map to *HookConfig Feb 17, 2024
@gmlewis gmlewis merged commit 7342217 into google:master Feb 17, 2024
5 checks passed
@gmlewis gmlewis removed the NeedsReview PR is awaiting a review before merging. label Feb 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hook should contain Config of type HookConfig
3 participants