Skip to content
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

✨ Initial experimental Azure DevOps client #4377

Merged
merged 5 commits into from
Nov 12, 2024

Conversation

JamieMagee
Copy link
Contributor

What kind of change does this PR introduce?

This PR introduces early experimental support for Azure DevOps. It's the MVP for a scorecard scan to complete without panicing.

What is the current behavior?

No Support for Azure DevOps repository scanning

What is the new behavior (if this is a feature change)?**

This is a followup from #4178, and the next step for #4177. It is the minimal client implementation for Azure DevOps repository support.

I'd like to get feedback at this stage, before I implement the remainder of the client.

  • Tests for the changes have been added (for bug fixes/features)

Which issue(s) this PR fixes

Continued work on #4177

Special notes for your reviewer

This change is currenltly gated behind the SCORECARD_EXPERIMENTAL environment variable, same as the initial GitLab client was.

Does this PR introduce a user-facing change?

For user-facing changes, please add a concise, human-readable release note to
the release-note

(In particular, describe what changes users might need to make in their
application as a result of this pull request.)

Initial experimental Azure DevOps client

@JamieMagee JamieMagee requested a review from a team as a code owner October 9, 2024 05:10
@JamieMagee JamieMagee requested review from justaugustus and spencerschrock and removed request for a team October 9, 2024 05:10
Copy link

codecov bot commented Oct 9, 2024

Codecov Report

Attention: Patch coverage is 8.89680% with 256 lines in your changes missing coverage. Please review.

Project coverage is 59.36%. Comparing base (353ed60) to head (ecf43e0).
Report is 53 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4377      +/-   ##
==========================================
- Coverage   66.80%   59.36%   -7.44%     
==========================================
  Files         230      216      -14     
  Lines       16602    16371     -231     
==========================================
- Hits        11091     9719    -1372     
- Misses       4808     5948    +1140     
- Partials      703      704       +1     

Copy link
Member

@spencerschrock spencerschrock left a comment

Choose a reason for hiding this comment

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

It's the MVP for a scorecard scan to complete without panicing.

panicing, or completing without runtime errors? What's the manual testing of this look like for the maintainers? Are there any world-public azuredevops repos?

I'd like to get feedback at this stage, before I implement the remainder of the client.

In general, the pattern is good. You copied the existing structure, and you're working through it piece by piece.

Did you plan on finishing the client in this PR or followups? Breaking it up in separate PRs may be easier. Your first approach was "run scorecard without panicing", but I think a good next step is "run a specific check"

So start with something like go run main.go --repo <blah> --checks Maintained, which just needs some info about the repo, commits, and issues. Code-Review is probably in this group too.

Then you can move on to checks which need file content (Binary-Artifacts may be a good one?), and then some of the more involved ones that rely on CI/CD data etc.

clients/azuredevopsrepo/client.go Outdated Show resolved Hide resolved
clients/azuredevopsrepo/client.go Outdated Show resolved Hide resolved
clients/azuredevopsrepo/client.go Outdated Show resolved Hide resolved
clients/azuredevopsrepo/repo.go Show resolved Hide resolved
clients/azuredevopsrepo/client.go Outdated Show resolved Hide resolved
clients/azuredevopsrepo/commits.go Outdated Show resolved Hide resolved
clients/azuredevopsrepo/commits.go Outdated Show resolved Hide resolved
clients/azuredevopsrepo/commits.go Outdated Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
@spencerschrock
Copy link
Member

There's a lot of linter errors that I'm sure you're waiting to resolve until after your approach got reviewed. So I didn't review any of that sort of thing, happy to give tips if desired.

@JamieMagee
Copy link
Contributor Author

panicing, or completing without runtime errors? What's the manual testing of this look like for the maintainers? Are there any world-public azuredevops repos?

Completing without runtime errors. Here's the output, running against https://dev.azure.com/jamiemagee/jamiemagee/_git/jamiemagee, after my most recent changes:

Details
Aggregate score: 0.0 / 10

Check scores:
|--------|------------------------|---------------------------------------|-----------------------------------------------------------------------------------|
| SCORE  |          NAME          |                REASON                 |                             DOCUMENTATION/REMEDIATION                             |
|--------|------------------------|---------------------------------------|-----------------------------------------------------------------------------------|
| ?      | Binary-Artifacts       | internal error: error during          | https://github.com/ossf/scorecard/blob/main/docs/checks.md#binary-artifacts       |
|        |                        | ListFiles: unsupported feature        |                                                                                   |
|--------|------------------------|---------------------------------------|-----------------------------------------------------------------------------------|
| ?      | Branch-Protection      | internal error: unsupported           | https://github.com/ossf/scorecard/blob/main/docs/checks.md#branch-protection      |
|        |                        | feature                               |                                                                                   |
|--------|------------------------|---------------------------------------|-----------------------------------------------------------------------------------|
| ?      | CI-Tests               | no pull request found                 | https://github.com/ossf/scorecard/blob/main/docs/checks.md#ci-tests               |
|--------|------------------------|---------------------------------------|-----------------------------------------------------------------------------------|
| 0 / 10 | CII-Best-Practices     | no effort to earn an OpenSSF          | https://github.com/ossf/scorecard/blob/main/docs/checks.md#cii-best-practices     |
|        |                        | best practices badge detected         |                                                                                   |
|--------|------------------------|---------------------------------------|-----------------------------------------------------------------------------------|
| 0 / 10 | Code-Review            | Found 0/1 approved changesets         | https://github.com/ossf/scorecard/blob/main/docs/checks.md#code-review            |
|        |                        | -- score normalized to 0              |                                                                                   |
|--------|------------------------|---------------------------------------|-----------------------------------------------------------------------------------|
| ?      | Contributors           | internal error:                       | https://github.com/ossf/scorecard/blob/main/docs/checks.md#contributors           |
|        |                        | Client.Repositories.ListContributors: |                                                                                   |
|        |                        | unsupported feature                   |                                                                                   |
|--------|------------------------|---------------------------------------|-----------------------------------------------------------------------------------|
| ?      | Dangerous-Workflow     | internal error: error during          | https://github.com/ossf/scorecard/blob/main/docs/checks.md#dangerous-workflow     |
|        |                        | ListFiles: unsupported feature        |                                                                                   |
|--------|------------------------|---------------------------------------|-----------------------------------------------------------------------------------|
| ?      | Dependency-Update-Tool | internal error: error during          | https://github.com/ossf/scorecard/blob/main/docs/checks.md#dependency-update-tool |
|        |                        | ListFiles: unsupported feature        |                                                                                   |
|--------|------------------------|---------------------------------------|-----------------------------------------------------------------------------------|
| ?      | Fuzzing                | internal error: error during          | https://github.com/ossf/scorecard/blob/main/docs/checks.md#fuzzing                |
|        |                        | ListFiles: unsupported feature        |                                                                                   |
|--------|------------------------|---------------------------------------|-----------------------------------------------------------------------------------|
| ?      | License                | internal error:                       | https://github.com/ossf/scorecard/blob/main/docs/checks.md#license                |
|        |                        | fileparser.OnAllFilesDo: error        |                                                                                   |
|        |                        | during ListFiles: unsupported         |                                                                                   |
|        |                        | feature                               |                                                                                   |
|--------|------------------------|---------------------------------------|-----------------------------------------------------------------------------------|
| ?      | Maintained             | internal error: unsupported           | https://github.com/ossf/scorecard/blob/main/docs/checks.md#maintained             |
|        |                        | feature                               |                                                                                   |
|--------|------------------------|---------------------------------------|-----------------------------------------------------------------------------------|
| ?      | Packaging              | packaging workflow not                | https://github.com/ossf/scorecard/blob/main/docs/checks.md#packaging              |
|        |                        | detected                              |                                                                                   |
|--------|------------------------|---------------------------------------|-----------------------------------------------------------------------------------|
| ?      | Pinned-Dependencies    | internal error: error during          | https://github.com/ossf/scorecard/blob/main/docs/checks.md#pinned-dependencies    |
|        |                        | ListFiles: unsupported feature        |                                                                                   |
|--------|------------------------|---------------------------------------|-----------------------------------------------------------------------------------|
| ?      | SAST                   | internal error: error during          | https://github.com/ossf/scorecard/blob/main/docs/checks.md#sast                   |
|        |                        | ListFiles: unsupported feature        |                                                                                   |
|--------|------------------------|---------------------------------------|-----------------------------------------------------------------------------------|
| ?      | SBOM                   | internal error:                       | https://github.com/ossf/scorecard/blob/main/docs/checks.md#sbom                   |
|        |                        | RepoClient.ListReleases:              |                                                                                   |
|        |                        | unsupported feature                   |                                                                                   |
|--------|------------------------|---------------------------------------|-----------------------------------------------------------------------------------|
| ?      | Security-Policy        | internal error: error during          | https://github.com/ossf/scorecard/blob/main/docs/checks.md#security-policy        |
|        |                        | ListFiles: unsupported feature        |                                                                                   |
|--------|------------------------|---------------------------------------|-----------------------------------------------------------------------------------|
| ?      | Signed-Releases        | internal error: unsupported           | https://github.com/ossf/scorecard/blob/main/docs/checks.md#signed-releases        |
|        |                        | feature                               |                                                                                   |
|--------|------------------------|---------------------------------------|-----------------------------------------------------------------------------------|
| ?      | Token-Permissions      | internal error: error during          | https://github.com/ossf/scorecard/blob/main/docs/checks.md#token-permissions      |
|        |                        | ListFiles: unsupported feature        |                                                                                   |
|--------|------------------------|---------------------------------------|-----------------------------------------------------------------------------------|
| ?      | Vulnerabilities        | internal error:                       | https://github.com/ossf/scorecard/blob/main/docs/checks.md#vulnerabilities        |
|        |                        | RepoClient.LocalPath:                 |                                                                                   |
|        |                        | unsupported feature                   |                                                                                   |
|--------|------------------------|---------------------------------------|-----------------------------------------------------------------------------------|
| ?      | Webhooks               | internal error: internal error:       | https://github.com/ossf/scorecard/blob/main/docs/checks.md#webhooks               |
|        |                        | Client.Repositories.ListWebhooks      |                                                                                   |
|--------|------------------------|---------------------------------------|-----------------------------------------------------------------------------------|

Did you plan on finishing the client in this PR or followups? Breaking it up in separate PRs may be easier. Your first approach was "run scorecard without panicing", but I think a good next step is "run a specific check"

So start with something like go run main.go --repo <blah> --checks Maintained, which just needs some info about the repo, commits, and issues. Code-Review is probably in this group too.

Then you can move on to checks which need file content (Binary-Artifacts may be a good one?), and then some of the more involved ones that rely on CI/CD data etc.

I think I'd like to finish the client in a followup PR if that's possible? I think it'd be easier for you to review in smaller chunks instead of one, big bang, pull request.

I plan to attend the scorecard APAC meeting on Thursday to gauge interest as well.

There's a lot of linter errors that I'm sure you're waiting to resolve until after your approach got reviewed. So I didn't review any of that sort of thing, happy to give tips if desired.

I'll tackle those in the next day or two.

Thanks so much for the review!

Copy link
Member

@spencerschrock spencerschrock left a comment

Choose a reason for hiding this comment

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

I think I'd like to finish the client in a followup PR if that's possible? I think it'd be easier for you to review in smaller chunks

Agreed. I went ahead and approved the boilerplate, pending linter fixes.

There's a lot of linter errors that I'm sure you're waiting to resolve until after your approach got reviewed. So I didn't review any of that sort of thing, happy to give tips if desired.

I'll tackle those in the next day or two.

make fix-linter may help with some auto-generated fixes.
Anything that complains about struct alignment you can usually fix with

go install golang.org/x/tools/go/analysis/passes/fieldalignment/cmd/fieldalignment
fieldalignment -fix ./clients/azuredevopsrepo

Happy to help with any others as well.

clients/azuredevopsrepo/client.go Outdated Show resolved Hide resolved
clients/azuredevopsrepo/client.go Outdated Show resolved Hide resolved
clients/azuredevopsrepo/client.go Outdated Show resolved Hide resolved
clients/azuredevopsrepo/client.go Show resolved Hide resolved
clients/azuredevopsrepo/commits.go Outdated Show resolved Hide resolved
clients/azuredevopsrepo/repo.go Outdated Show resolved Hide resolved
@spencerschrock
Copy link
Member

spencerschrock commented Oct 14, 2024

also your latest commit wasn't DCO'd. that will also be a blocker. The checkrun will have remediation info:
https://github.com/ossf/scorecard/pull/4377/checks?check_run_id=31438639753

@JamieMagee
Copy link
Contributor Author

@spencerschrock I think I've addressed all your comments. If so, this should be ready to merge.

Copy link

github-actions bot commented Nov 2, 2024

This pull request has been marked stale because it has been open for 10 days with no activity

@github-actions github-actions bot added the Stale label Nov 2, 2024
@spencerschrock
Copy link
Member

@spencerschrock I think I've addressed all your comments. If so, this should be ready to merge.

I've been out on the office on personal time, will take a look in the next day or two.

Copy link
Member

@spencerschrock spencerschrock left a comment

Choose a reason for hiding this comment

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

Just one small clarification to what I meant by "caching".

clients/azuredevopsrepo/client.go Show resolved Hide resolved
clients/azuredevopsrepo/client.go Outdated Show resolved Hide resolved
Signed-off-by: Jamie Magee <[email protected]>
Signed-off-by: Jamie Magee <[email protected]>
Signed-off-by: Jamie Magee <[email protected]>
Signed-off-by: Jamie Magee <[email protected]>
@spencerschrock spencerschrock merged commit fee8bcf into ossf:main Nov 12, 2024
37 of 38 checks passed
@justaugustus
Copy link
Member

Wooo, thanks for this contribution, @JamieMagee!

@JamieMagee JamieMagee deleted the azure-devops-client branch November 14, 2024 04:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants