-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat!: Add enterprise security configurations, update API fields #3812
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
base: master
Are you sure you want to change the base?
Conversation
|
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. |
|
sorry forgot to add the generated files after my latest change.. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3812 +/- ##
==========================================
+ Coverage 92.27% 92.32% +0.05%
==========================================
Files 192 193 +1
Lines 13896 13988 +92
==========================================
+ Hits 12823 12915 +92
Misses 884 884
Partials 189 189 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
gmlewis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please let me know if I should drop the commit with breaking changes and what you think about the other commits
I think that is fine, thanks. Please address the few minor suggestions I've made, then we should be ready for a second LGTM+Approval from any other contributor to this repo before merging.
cc: @stevehipwell - @alexandear - @zyfy29
| // GitHub API docs: https://docs.github.com/rest/code-security/configurations#create-a-code-security-configuration-for-an-enterprise | ||
| // | ||
| //meta:operation POST /enterprises/{enterprise}/code-security/configurations | ||
| func (s *EnterpriseService) CreateCodeSecurityConfiguration(ctx context.Context, enterprise string, c *CodeSecurityConfiguration) (*CodeSecurityConfiguration, *Response, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| func (s *EnterpriseService) CreateCodeSecurityConfiguration(ctx context.Context, enterprise string, c *CodeSecurityConfiguration) (*CodeSecurityConfiguration, *Response, error) { | |
| func (s *EnterpriseService) CreateCodeSecurityConfiguration(ctx context.Context, enterprise string, config CodeSecurityConfiguration) (*CodeSecurityConfiguration, *Response, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think should be a separate struct (fields name and description are required):
| func (s *EnterpriseService) CreateCodeSecurityConfiguration(ctx context.Context, enterprise string, c *CodeSecurityConfiguration) (*CodeSecurityConfiguration, *Response, error) { | |
| func (s *EnterpriseService) CreateCodeSecurityConfiguration(ctx context.Context, enterprise string, config CreateCodeSecurityConfiguration) (*CodeSecurityConfiguration, *Response, error) { |
type CreateCodeSecurityConfiguration struct {
Name string `json:"name"`
Desription string `json:"description"`
// all other optional fields
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed that in the original struct since this also aplies to the org endpoints
| // GitHub API docs: https://docs.github.com/rest/code-security/configurations#get-code-security-configurations-for-an-enterprise | ||
| // | ||
| //meta:operation GET /enterprises/{enterprise}/code-security/configurations | ||
| func (s *EnterpriseService) GetCodeSecurityConfigurations(ctx context.Context, enterprise string) ([]*CodeSecurityConfiguration, *Response, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't there be something generic that could also used for the org endpoint (and possibly others)? I only see pagination options for projects currently
| // GitHub API docs: https://docs.github.com/rest/code-security/configurations#create-a-code-security-configuration-for-an-enterprise | ||
| // | ||
| //meta:operation POST /enterprises/{enterprise}/code-security/configurations | ||
| func (s *EnterpriseService) CreateCodeSecurityConfiguration(ctx context.Context, enterprise string, c *CodeSecurityConfiguration) (*CodeSecurityConfiguration, *Response, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think should be a separate struct (fields name and description are required):
| func (s *EnterpriseService) CreateCodeSecurityConfiguration(ctx context.Context, enterprise string, c *CodeSecurityConfiguration) (*CodeSecurityConfiguration, *Response, error) { | |
| func (s *EnterpriseService) CreateCodeSecurityConfiguration(ctx context.Context, enterprise string, config CreateCodeSecurityConfiguration) (*CodeSecurityConfiguration, *Response, error) { |
type CreateCodeSecurityConfiguration struct {
Name string `json:"name"`
Desription string `json:"description"`
// all other optional fields
}
BREAKING CHANGE:
AttachCodeSecurityConfigurationsToRepositoriesis nowAttachCodeSecurityConfigurationToRepositories.please let me know if I should drop the commit with breaking changes and what you think about the other commits