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
Add support for Code Scanning API #1545
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1545 +/- ##
==========================================
- Coverage 67.92% 67.90% -0.02%
==========================================
Files 94 95 +1
Lines 8670 8722 +52
==========================================
+ Hits 5889 5923 +34
- Misses 1880 1892 +12
- Partials 901 907 +6
Continue to review full report at Codecov.
|
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.
Very nice, @nightlark ! I like this a lot!
A made a few comments... please let me know what you think.
github/code-scanning.go
Outdated
Tool *string `json:"tool,omitempty"` | ||
CreatedAt *time.Time `json:"created_at,omitempty"` | ||
Open *bool `json:"open,omitempty"` | ||
ClosedBy *string `json:"closed_by,omitempty"` |
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.
Might ClosedBy
be a *User
? Maybe you could ask support@github.com to find out?
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.
If it's the same as ClosedBy
for issues, yes. Will confirm with GitHub support.
github/code-scanning.go
Outdated
RuleSeverity *string `json:"rule_severity,omitempty"` | ||
RuleDescription *string `json:"rule_description,omitempty"` | ||
Tool *string `json:"tool,omitempty"` | ||
CreatedAt *time.Time `json:"created_at,omitempty"` |
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 believe we are trying to standardize on *Timestamp
in this repo instead of *time.Time
.
Here, and a few lines below for ClosedAt
.
// You must use an access token with the security_events scope to use this endpoint. | ||
// GitHub Apps must have the security_events read permission to use this endpoint. | ||
// | ||
// The security alert_id is the number at the end of the security alert's URL. |
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.
Would it be possible to make this a little more accessible from ListAlertsForRepo
so that users of the library don't have to parse it out themselves?
I'm thinking we could add a helper method like:
func (a *Alert) ID() int64 {
...
}
that would then do the parsing for the user of this repo? (If you agree, then this comment can be updated.)
Thoughts?
(Note that we want this to return 0
if a==nil
or if parsing fails, and not panic. 😄 )
(Alternatively, we could change the signature to return (int64, error)
, but I prefer the above since they could parse things themselves if they want.)
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.
That's a good idea. I added a helper function that checks Alert.HTMLURL first for something resembling a url with an ID at the end (split at last forward slash, if no forward slash then tries to split at the last trailing slash in Alert.URL), then converts whatever was found to an int64. Checking HTMLURL first was because the example given for getting the alert id used HTMLURL.
Do you think it's worth checking both HTMLURL and URL, or that there should be a more extensive check to see if the URL is valid before trying to convert whatever was after the last slash to an int64?
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.
Thank you, @nightlark!
Just a few tweaks, please.
github/code-scanning.go
Outdated
// Check if HTMLURL is valid | ||
if a.HTMLURL != nil { | ||
s = *a.HTMLURL | ||
} |
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 replace lines 40-47 with:
s := a.GetHTMLURL()
And just for future reference, lines 41 and 42 are not idiomatic Go. They would be var s string
and i := -1
respectively, but there is no need to initialize i
up there when it can be initialized in the if
where it is used. Also, since it is only used within the if
it makes sense to not expose its scope beyond the if
.
// Both HTMLURL and URL were invalid and had no ID | ||
return 0 | ||
} | ||
} |
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 don't think we need to use URL
as a fallback. In my experience, HTMLURL
is always populated. That will simplify this method significantly.
Let's go ahead and delete lines 52-63.
github/code-scanning.go
Outdated
if id, err := strconv.ParseInt(s, 10, 64); err == nil { | ||
return id | ||
} | ||
return 0 |
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.
Idiomatic Go would be to make the "happy path" the main path so that you can look at the end of the function to see the result that you want, and all other returns are typically error conditions. So that would mean that lines 67-70 would be:
id, err := strconv.ParseInt(s, 10, 64)
if err != nil {
return 0
}
return id
github/code-scanning_test.go
Outdated
@@ -14,6 +14,56 @@ import ( | |||
"time" | |||
) | |||
|
|||
func TestActionsService_Alert_ID(t *testing.T) { | |||
// Test: nil Alert ID == 0 | |||
a := (*Alert)(nil) |
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.
Idiomatic Go would be (since zero values are useful):
var a *Alert
github/code-scanning_test.go
Outdated
// Test: nil Alert ID == 0 | ||
a := (*Alert)(nil) | ||
id := a.ID() | ||
want := int64(0) |
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.
This is fine, but could also be:
var want int64 = 0
or even
var want int64
github/code-scanning.go
Outdated
} | ||
|
||
// Check HTMLURL for an ID to parse first, since it was used in the example | ||
if i = strings.LastIndex(s, "/"); i != -1 { |
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.
Whups, I took a dinner break and forgot to comment that this would be:
if i := strings.LastIndex(s, "/"); i >= 0 {
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.
Thank you, @nightlark!
LGTM.
Awaiting second LGTM before merging.
Should we wait until GitHub support responds with an answer for the |
Hmmm... good question. We already have a breaking change in the queue, unfortunately, immediately after the release of |
Updating when we hear back if needed would be okay. If I hear back before the next LGTM I'll update the PR. |
Here's the response from GH support on the
|
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.
Thank you, @nightlark !
LGTM.
Awaiting second LGTM before merging.
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.
LGTM 👌
Thank you, @wesleimp ! |
Adds a Code Scanning service with functions for getting code scanning alerts.
Docs: https://developer.github.com/v3/code-scanning/