Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: Support Code Security Configurations API #3319

Merged
merged 7 commits into from
Oct 14, 2024

Conversation

maditya
Copy link
Contributor

@maditya maditya commented Oct 9, 2024

This PR implements Code Security Configurations APIs as defined at https://docs.github.com/en/rest/code-security/configurations?apiVersion=2022-11-28

Fixes #3211

Copy link

google-cla bot commented Oct 9, 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.

@maditya maditya force-pushed the codesecurity-config branch from 983763b to c8041cd Compare October 9, 2024 19:39
Copy link

codecov bot commented Oct 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.36%. Comparing base (2b8c7fa) to head (e640711).
Report is 150 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3319      +/-   ##
==========================================
- Coverage   97.72%   93.36%   -4.36%     
==========================================
  Files         153      172      +19     
  Lines       13390    11843    -1547     
==========================================
- Hits        13085    11057    -2028     
- Misses        215      692     +477     
- Partials       90       94       +4     

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

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!
Just a few tweaks, and please remember to run go generate ./... as mentioned in step 4 of CONTRIBUTING.md and push (not force-push) the changes to this PR.
We then should be ready for a second LGTM+Approval from any other contributor to this repo before merging.

github/orgs_codesecurity_configurations.go Show resolved Hide resolved

// DependencyGraphAutosubmitActionOptions represents the options for the DependencyGraphAutosubmitAction.
type DependencyGraphAutosubmitActionOptions struct {
LabeledRunners bool `json:"labeled_runners,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
LabeledRunners bool `json:"labeled_runners,omitempty"`
LabeledRunners *bool `json:"labeled_runners,omitempty"`

// GitHub API docs: https://docs.github.com/en/rest/code-security/configurations#get-the-code-security-configuration-associated-with-a-repository
//
//meta:operation GET /repos/{owner}/{repo}/code-security-configuration
func (s *OrganizationsService) GetCodeSecurityConfigurationForRepository(ctx context.Context, org string, repo string) (*RepositoryCodeSecurityConfiguration, *Response, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
func (s *OrganizationsService) GetCodeSecurityConfigurationForRepository(ctx context.Context, org string, repo string) (*RepositoryCodeSecurityConfiguration, *Response, error) {
func (s *OrganizationsService) GetCodeSecurityConfigurationForRepository(ctx context.Context, org, repo string) (*RepositoryCodeSecurityConfiguration, *Response, error) {

@gmlewis gmlewis added the NeedsReview PR is awaiting a review before merging. label Oct 9, 2024
@maditya
Copy link
Contributor Author

maditya commented Oct 9, 2024

@gmlewis Thanks!
I tried and fixed go generate ./... which updated a bunch of other files.
But metadata script is failing -

ambiguous operation "GET /user/{user_id}" could match any of: [GET /user/{account_id} GET /user/{user_id}]
github/github.go:8: running "../script/metadata.sh": exit status 1

Any idea how to address it? Doesn't seem related to the new API.

@gmlewis
Copy link
Collaborator

gmlewis commented Oct 10, 2024

Any idea how to address it?

Hmmm... a lot more files changed than I expected.
It looks like you may have locally updated the OpenAPI specs, which is typically done by a maintainer of the repo.

We have two options:

  1. revert the changes to all the files except your original two, then run go generate ./...
  2. wait for another PR to update all the OpenAPI stuff and fix all the errors you are seeing

I think option 1 is probably better since option 2 may take a while, and I'm currently monitoring a hurricane locally.

@maditya
Copy link
Contributor Author

maditya commented Oct 10, 2024

revert the changes to all the files except your original two, then run go generate ./...

Done. Note that go generate ./... updates some other files, namely github/admin_users.go, github/github-accessors.go and github/github-accessors_test.go and will fail until openapi_operations.yaml is updated. I get these errors:

could not find operation "GET /orgs/{org}/code-security/configurations" in openapi_operations.yaml
could not find operation "POST /orgs/{org}/code-security/configurations" in openapi_operations.yaml
could not find operation "GET /orgs/{org}/code-security/configurations/defaults" in openapi_operations.yaml
could not find operation "DELETE /orgs/{org}/code-security/configurations/detach" in openapi_operations.yaml
could not find operation "GET /orgs/{org}/code-security/configurations/{configuration_id}" in openapi_operations.yaml
could not find operation "PATCH /orgs/{org}/code-security/configurations/{configuration_id}" in openapi_operations.yaml
could not find operation "DELETE /orgs/{org}/code-security/configurations/{configuration_id}" in openapi_operations.yaml
could not find operation "POST /orgs/{org}/code-security/configurations/{configuration_id}/attach" in openapi_operations.yaml
could not find operation "PUT /orgs/{org}/code-security/configurations/{configuration_id}/defaults" in openapi_operations.yaml
could not find operation "GET /orgs/{org}/code-security/configurations/{configuration_id}/repositories" in openapi_operations.yaml
could not find operation "GET /repos/{owner}/{repo}/code-security-configuration" in openapi_operations.yaml
github/github.go:8: running "../script/metadata.sh": exit status 1

@gmlewis
Copy link
Collaborator

gmlewis commented Oct 10, 2024

Note that go generate ./... updates some other files

That is really strange.
go generate ./... should only update three files (as needed):

  • github/github-accessors.go
  • github/github-accessors_test.go
  • github/github-stringify_test.go

I'm going to make a new branch locally with just your changes to your two files and run go generate ./... and see what happens for me and report back.

@gmlewis
Copy link
Collaborator

gmlewis commented Oct 10, 2024

I'm going to make a new branch locally with just your changes to your two files and run go generate ./... and see what happens for me and report back.

Ah, I see what is going on now. You are adding support for new operations not yet defined in our local OpenAPI yaml.

This section in CONTRIBUTING.md is pertinent to you:

openapi_operations.yaml

Most contributors won't need to interact with openapi_operations.yaml, but it
may be useful to know what it is. Its sections are:
...

So you can either attempt to update it yourself by running script/metadata.sh update-openapi and then work out any issues you find, or I can do that in a separate PR. Which would you prefer?

@gmlewis
Copy link
Collaborator

gmlewis commented Oct 10, 2024

EDIT: I've gone ahead and created #3322 which you should be able to merge into your PR (from master once I merge it EDIT: merged), which should make the rest of this PR easier to finish.

It looks like (in addition to the code generation), the tests need to have more coverage by using existing test helper methods like testBadOptions and testNewRequestAndDoFailure. You can see these in the GitHub UI.

@maditya
Copy link
Contributor Author

maditya commented Oct 10, 2024

thanks! go generate ./... works as expected now. I added remaining tests.

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.

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 14, 2024
@gmlewis
Copy link
Collaborator

gmlewis commented Oct 14, 2024

Thank you, @tomfeigin !
Merging.

@gmlewis gmlewis merged commit b1615cb into google:master Oct 14, 2024
6 of 7 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for new Code Security Configurations API
3 participants