Add support for Code Scanning API by nightlark · Pull Request #1545 · google/go-github

nightlark · 2020-05-30T17:40:11Z

Adds a Code Scanning service with functions for getting code scanning alerts.

Docs: https://developer.github.com/v3/code-scanning/

codecov · 2020-05-30T17:41:31Z

Codecov Report

Merging #1545 into master will decrease coverage by 0.01%.
The diff coverage is 66.66%.

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

Impacted Files	Coverage Δ
github/authorizations.go	`71.57% <ø> (ø)`
github/code-scanning.go	`65.90% <65.90%> (ø)`
github/github.go	`89.67% <100.00%> (+0.02%)`	⬆️
github/orgs.go	`69.69% <0.00%> (ø)`
github/users.go	`53.39% <0.00%> (ø)`
github/actions_workflow_runs.go	`57.57% <0.00%> (ø)`
github/repos_deployments.go	`56.31% <0.00%> (+0.06%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 407d736...0b014cc. Read the comment docs.

gmlewis

Very nice, @nightlark ! I like this a lot!
A made a few comments... please let me know what you think.

gmlewis · 2020-05-30T21:17:50Z

+	Tool            *string    `json:"tool,omitempty"`
+	CreatedAt       *time.Time `json:"created_at,omitempty"`
+	Open            *bool      `json:"open,omitempty"`
+	ClosedBy        *string    `json:"closed_by,omitempty"`


Might ClosedBy be a *User ? Maybe you could ask support@github.com to find out?

If it's the same as ClosedBy for issues, yes. Will confirm with GitHub support.

gmlewis · 2020-05-30T21:18:29Z

+	RuleSeverity    *string    `json:"rule_severity,omitempty"`
+	RuleDescription *string    `json:"rule_description,omitempty"`
+	Tool            *string    `json:"tool,omitempty"`
+	CreatedAt       *time.Time `json:"created_at,omitempty"`


I believe we are trying to standardize on *Timestamp in this repo instead of *time.Time.
Here, and a few lines below for ClosedAt.

gmlewis · 2020-05-30T21:21:57Z

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


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

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?

gmlewis

Thank you, @nightlark!
Just a few tweaks, please.

gmlewis · 2020-05-30T23:43:55Z

+	// Check if HTMLURL is valid
+	if a.HTMLURL != nil {
+		s = *a.HTMLURL
+	}


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.

gmlewis · 2020-05-30T23:45:41Z

+			// Both HTMLURL and URL were invalid and had no ID
+			return 0
+		}
+	}


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.

gmlewis · 2020-05-31T00:02:56Z

+	if id, err := strconv.ParseInt(s, 10, 64); err == nil {
+		return id
+	}
+	return 0


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

gmlewis · 2020-05-31T00:04:11Z


+func TestActionsService_Alert_ID(t *testing.T) {
+	// Test: nil Alert ID == 0
+	a := (*Alert)(nil)


Idiomatic Go would be (since zero values are useful):

var a *Alert

gmlewis · 2020-05-31T00:05:07Z

+	// Test: nil Alert ID == 0
+	a := (*Alert)(nil)
+	id := a.ID()
+	want := int64(0)


This is fine, but could also be:

var want int64 = 0

or even

var want int64

gmlewis · 2020-05-31T00:07:03Z

+	}
+
+	// Check HTMLURL for an ID to parse first, since it was used in the example
+	if i = strings.LastIndex(s, "/"); i != -1 {


Whups, I took a dinner break and forgot to comment that this would be:

if i := strings.LastIndex(s, "/"); i >= 0 {

gmlewis

Thank you, @nightlark!
LGTM.

Awaiting second LGTM before merging.

nightlark · 2020-05-31T03:36:33Z

Should we wait until GitHub support responds with an answer for the closed_by field format, leave it as is for now, or switch it to *User? Since the example has it as null, I'd guess that *User is more likely the return type than *string*.

gmlewis · 2020-05-31T12:43:00Z

Hmmm... good question. We already have a breaking change in the queue, unfortunately, immediately after the release of v32.0.0, so we could proceed with what we have here and then update it (if necessary) when we hear back from GitHub tech support... all before the release of v33.0.0. I'm fine with that if you are OK with that. But if you would prefer to wait, I'm totally fine with that too.

nightlark · 2020-05-31T16:13:20Z

Updating when we hear back if needed would be okay. If I hear back before the next LGTM I'll update the PR.

nightlark · 2020-06-03T05:36:29Z

Here's the response from GH support on the closed_by field:

I've had a look at our schema for that API response and, when the alert is closed, we'll return a user object for whoever closed it.

I don't have an example but it would be the same response as you see for user for this pull request:

https://developer.github.com/v3/pulls/#response-2

gmlewis

Thank you, @nightlark !
LGTM.

Awaiting second LGTM before merging.

wesleimp

LGTM 👌

gmlewis · 2020-06-17T01:20:03Z

Thank you, @wesleimp !
Merging.

nightlark added 2 commits May 30, 2020 08:38

Work-in progress code-scanning

4f98ea0

Add code scanning tests and fix bugs

6051648

googlebot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label May 30, 2020

gmlewis requested changes May 30, 2020

View reviewed changes

nightlark added 2 commits May 30, 2020 14:48

Replace time.Time with Timestamp

8278503

Add helper to get an Alert ID from one of the URLs

0d7062f

gmlewis requested changes May 31, 2020

View reviewed changes

gmlewis reviewed May 31, 2020

View reviewed changes

Make suggested changes to Alert.ID()

a5f2d78

gmlewis approved these changes May 31, 2020

View reviewed changes

gmlewis requested review from gauntface and wesleimp May 31, 2020 03:18

Switch ClosedBy field to type User based on GH support response

cb1bbde

gmlewis approved these changes Jun 3, 2020

View reviewed changes

wesleimp approved these changes Jun 16, 2020

View reviewed changes

Fix comment for auto-generated godocs

0b014cc

gmlewis merged commit 2fe941a into google:master Jun 17, 2020

nightlark deleted the code-scanning-api branch August 15, 2020 15:31

n1lesh pushed a commit to n1lesh/go-github that referenced this pull request Oct 2, 2020

Add support for Code Scanning API (google#1545)

589909b

Conversation

nightlark commented May 30, 2020

Uh oh!

codecov Bot commented May 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gmlewis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmlewis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmlewis May 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmlewis left a comment

Choose a reason for hiding this comment

Uh oh!

nightlark commented May 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmlewis commented May 31, 2020

Uh oh!

nightlark commented May 31, 2020

Uh oh!

nightlark commented Jun 3, 2020

Uh oh!

gmlewis left a comment

Choose a reason for hiding this comment

Uh oh!

wesleimp left a comment

Choose a reason for hiding this comment

Uh oh!

gmlewis commented Jun 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented May 30, 2020 •

edited

Loading

gmlewis May 31, 2020 •

edited

Loading

nightlark commented May 31, 2020 •

edited

Loading