Skip to content

Add CodeScanningService.ListAnalysesForRepo and CodeScanningService.GetAnalysis - #2210

Merged
gmlewis merged 5 commits into
google:masterfrom
yogur:code-scanning-analysis
Dec 3, 2021
Merged

Add CodeScanningService.ListAnalysesForRepo and CodeScanningService.GetAnalysis#2210
gmlewis merged 5 commits into
google:masterfrom
yogur:code-scanning-analysis

Conversation

@yogur

@yoguryogur commented Nov 26, 2021

Copy link
Copy Markdown
Contributor

Add ListAnalysesForRepo and GetAnalysis to CodeScanningService, including unit tests.

@yogur

yogur commented Nov 26, 2021

Copy link
Copy Markdown
ContributorAuthor

I just submitted a CLA.

@yogur

yogur commented Nov 26, 2021

Copy link
Copy Markdown
ContributorAuthor

I noticed that most of the unit tests are prefixed with "TestActionsService", shouldn't these unit tests be prefixed with "TestCodeScanningService" instead?

@codecov

codecovBot commented Nov 26, 2021•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #2210 (94d6ae9) into master (b26fa8f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@ Coverage Diff @@## master #2210 +/- ##
=======================================
Coverage 97.79% 97.80% =======================================
Files 112 112 Lines 10036 10078 +42 =======================================
+ Hits 9815 9857 +42 
Misses 154 154 Partials 67 67
Impacted FilesCoverage Δ
github/code-scanning.go100.00% <100.00%> (ø)
github/actions_workflow_runs.go100.00% <0.00%> (ø)
github/github.go97.62% <0.00%> (+0.02%)⬆️

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 b26fa8f...94d6ae9. Read the comment docs.

@gmlewis

gmlewis commented Nov 26, 2021

Copy link
Copy Markdown
Collaborator

I noticed that most of the unit tests are prefixed with "TestActionsService", shouldn't these unit tests be prefixed with "TestCodeScanningService" instead?

Good catch! Yes indeed, these should be changed. Would you mind addressing these changes in this PR?
Thank you, @yogur !

@googlebot rescan.

@google-clagoogle-claBot added cla: yes Indication that the PR author has signed a Google Contributor License Agreement. and removed cla: no labels Nov 26, 2021
gmlewis
gmlewis requested changes Nov 26, 2021
View reviewed changes

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you, @yogur !
I have a couple questions about this. See below.

Comment threadgithub/code-scanning.go Outdated
Comment threadgithub/code-scanning.go Outdated
Comment threadgithub/code-scanning.go Outdated
// Analysis represents an individual GitHub Code Scanning Analysis on a single repository.
//
// GitHub API docs: https://docs.github.com/en/rest/reference/code-scanning#list-code-scanning-analyses-for-a-repository
type Analysis struct {

@gmlewisgmlewisNov 26, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can this be merged with the existing SarifAnalysis struct?
If not, can we give it a more specific name?

@yoguryogurNov 28, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

It can't because SarifAnalysis is used for POST request body params, while Analysis is used to unmarshal json response of listing code scanning analysis instances.

I thought about renaming to to ScanningAnalysis or CodeScanningAnalysis but I felt that was redundant with the service name. CodeScanningService. What are your thoughts on this?

@gmlewisgmlewisNov 28, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The problem is that this name is global throughout the package github namespace. So even though it is used within the CodeScanningService, it is really github.Analysis which is pretty general, so if you don't mind switching to ScanningAnalysis, I think that would be preferable. Thank you!

@yoguryogurNov 28, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Oh yes you're right. I updated it. Thanks for the review!

Comment threadgithub/code-scanning.go Outdated
Comment threadgithub/code-scanning.go Outdated
@yogur

yogur commented Nov 26, 2021

Copy link
Copy Markdown
ContributorAuthor

I noticed that most of the unit tests are prefixed with "TestActionsService", shouldn't these unit tests be prefixed with "TestCodeScanningService" instead?

Good catch! Yes indeed, these should be changed. Would you mind addressing these changes in this PR? Thank you, @yogur !

@googlebot rescan.

@gmlewis No problem! I committed the name change.

gmlewis
gmlewis approved these changes Nov 28, 2021
View reviewed changes

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you, @yogur !
LGTM.

Awaiting second LGTM before merging.

(Please note that ALL other contributors to this repo are welcome to provide the second PR review/comment/approval that we need for merging and that we are not waiting for any particular reviewer unless otherwise noted.)

cpanato
cpanato approved these changes Dec 3, 2021
View reviewed changes

@cpanatocpanato left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM
thanks!

@gmlewis

gmlewis commented Dec 3, 2021

Copy link
Copy Markdown
Collaborator

Thank you, @cpanato !
Merging.

@gmlewis
gmlewis merged commit 44ae9a1 into google:masterDec 3, 2021
@gmlewisgmlewis mentioned this pull request Jan 13, 2022
Merged
jlaportebot added a commit to jlaportebot/go-github that referenced this pull request Jun 28, 2026
@jlaportebot
Add CodeScanningService.ListAnalysesForRepo and CodeScanningService.G…
7502fdd
…etAnalysis (google#2210)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gmlewisgmlewisgmlewis approved these changes
@wesleimpwesleimpAwaiting requested review from wesleimp
+1 more reviewer
@cpanatocpanatocpanato approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yesIndication that the PR author has signed a Google Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@yogur@gmlewis@cpanato