Skip to content

Add BranchProtectionConfigurationEvent and SecretScanningAlertLocationEvent - #3332

Merged
gmlewis merged 3 commits into
google:masterfrom
maditya:missing-event
Oct 27, 2024
Merged

Add BranchProtectionConfigurationEvent and SecretScanningAlertLocationEvent#3332
gmlewis merged 3 commits into
google:masterfrom
maditya:missing-event

Conversation

@maditya

@madityamaditya commented Oct 23, 2024

Copy link
Copy Markdown
Contributor

No description provided.

@gmlewisgmlewis changed the title add event structs for branch_protection_configuration and secret_scan…Add BranchProtectionConfigurationEvent and SecretScanningAlertLocationEventOct 23, 2024
@codecov

codecovBot commented Oct 23, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.01%. Comparing base (2b8c7fa) to head (3073929).
Report is 160 commits behind head on master.

Additional details and impacted files
@@ Coverage Diff @@## master #3332 +/- ##
==========================================
- Coverage 97.72% 93.01% -4.71% 
==========================================
Files 153 172 +19 Lines 13390 14848 +1458 ==========================================
+ Hits 13085 13811 +726 - Misses 215 944 +729 - Partials 90 93 +3 

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

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you, @maditya.
To add new events, we need to make some more changes to the following files:

  • event_types_test.go
  • messages.go
  • messages_test.go

Please see #3258 for an example of the changes that are needed.
Thanks again!

@gmlewis

Copy link
Copy Markdown
Collaborator

Please fix the unit tests locally by running the commands in step 4 in CONTRIBUTING.md and then push (not force-push) the changes to this PR.

@maditya

Copy link
Copy Markdown
ContributorAuthor
--- FAIL: TestProjectsService_DeleteProjectCard (0.00s)
projects_test.go:611: Projects.DeleteProjectCard returned error: Delete "http://127.0.0.1:39961/api-v3/projects/columns/cards/1": net/http: HTTP/1.x transport connection broken: http: CloseIdleConnections called

@gmlewis this seems like an intermittent failure unrelated to the changes in this PR. Unable to reproduce it locally. Can we re-run the workflow?

"code_scanning_default_setup": "enabled"
}
}`)
w.WriteHeader(http.StatusOK)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The changes to this file seem wrong to me.
I think you can revert the changes to this file.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This was a warning which I fixed.

Per https://pkg.go.dev/net/http

If WriteHeader is not called explicitly, the first call to Write
will trigger an implicit WriteHeader(http.StatusOK).

So I think we should either keep this change or explicitly call w.WriteHeader(http.StatusOK) before we write to w. Any thoughts?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Oh, interesting, so this is unrelated to this PR. OK. I'm fine to keep the change. Thanks.

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you, @maditya !
LGTM.

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

@gmlewisgmlewis added the NeedsReview PR is awaiting a review before merging. label Oct 24, 2024

@tomfeigintomfeigin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@gmlewisgmlewis removed the NeedsReview PR is awaiting a review before merging. label Oct 27, 2024
@gmlewis

Copy link
Copy Markdown
Collaborator

Thank you, @tomfeigin !
Merging.

@gmlewis
gmlewis merged commit a812798 into google:masterOct 27, 2024
jlaportebot added a commit to jlaportebot/go-github that referenced this pull request Jun 28, 2026
Sign up for freeto 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.

3 participants

@maditya@gmlewis@tomfeigin