Skip to content

Add BranchProtectionConfigurationEvent and SecretScanningAlertLocationEvent #3332

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Oct 27, 2024
Merged

Add BranchProtectionConfigurationEvent and SecretScanningAlertLocationEvent #3332

merged 3 commits into from
Oct 27, 2024

Conversation

maditya
Copy link
Contributor

@maditya maditya commented Oct 23, 2024 •
edited by gmlewis
Loading

No description provided.

@gmlewis gmlewis changed the title add event structs for branch_protection_configuration and secret_scan… Add BranchProtectionConfigurationEvent and SecretScanningAlertLocationEvent Oct 23, 2024
@codecov Codecov
Copy link

codecov bot commented Oct 23, 2024 •
edited
Loading

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.

gmlewis
gmlewis requested changes Oct 23, 2024
View reviewed changes
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, @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
Collaborator

gmlewis commented Oct 24, 2024

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
Contributor Author

maditya commented Oct 24, 2024 

--- 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?

gmlewis
gmlewis reviewed Oct 24, 2024
View reviewed changes
github/orgs_codesecurity_configurations_test.go
@@ -382,7 +382,6 @@ func TestOrganizationsService_SetDefaultCodeSecurityConfiguration(t *testing.T)
"code_scanning_default_setup": "enabled"
}
}`)
w.WriteHeader(http.StatusOK)
Copy link
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
Contributor Author

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
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.

gmlewis
gmlewis approved these changes Oct 24, 2024
View reviewed changes
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, @maditya !
LGTM.

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

@gmlewis gmlewis added the NeedsReview PR is awaiting a review before merging. label Oct 24, 2024
tomfeigin
tomfeigin approved these changes Oct 27, 2024
View reviewed changes
Copy link
Contributor

@tomfeigin tomfeigin 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 gmlewis removed the NeedsReview PR is awaiting a review before merging. label Oct 27, 2024
@gmlewis
Copy link
Collaborator

gmlewis commented Oct 27, 2024

Thank you, @tomfeigin !
Merging.

@gmlewis gmlewis merged commit a812798 into google:master Oct 27, 2024
6 of 7 checks passed
@gmlewis gmlewis mentioned this pull request Nov 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmlewis gmlewis gmlewis approved these changes

@tomfeigin tomfeigin tomfeigin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@maditya @gmlewis @tomfeigin