🐛 do not allow unsupported ListReleases to fail a run
#4533
Conversation
JamieMagee
requested review from
justaugustus and
spencerschrock
and removed request for
a team
JamieMagee
temporarily deployed
to
integration-test
GitHub Actions
Inactive
Signed-off-by: Jamie Magee <[email protected]>
JamieMagee
force-pushed
the
jamiemagee/unsupported-list-releases
branch
from
191e138 to
218c95e
Compare
JamieMagee
temporarily deployed
to
integration-test
GitHub Actions
Inactive
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4533 +/- ##
==========================================
+ Coverage 66.80% 68.16% +1.36%
==========================================
Files 230 247 +17
Lines 16602 18641 +2039
==========================================
+ Hits 11091 12707 +1616
- Misses 4808 5089 +281
- Partials 703 845 +142 |
There was a problem hiding this comment.
Generally when handling clients.ErrUnsupportedFeature, we've done so in a way to enable partial results #3832 (and linked issues / PRs). As implemented, that's not happening here.
I think your PR may be aimed at a different problem though. When you're saying "fail a run", is the exit code of 1 interfering with the AZDO pipeline task?
| // Get release branches. | ||
| releases, err := c.ListReleases() | ||
| if err != nil { | ||
| if errors.Is(err, clients.ErrUnsupportedFeature) { | ||
| return checker.BranchProtectionsData{}, nil | ||
| } |
There was a problem hiding this comment.
we could still query branch protection of the default branch even if we can't determine the release branches.
Would something like this be better?:
// Get release branches.
releases, err := c.ListReleases()
if err != nil && !errors.Is(err, clients.ErrUnsupportedFeature) {
return checker.BranchProtectionsData{}, fmt.Errorf("%w", err)
}| releases, lerr := c.RepoClient.ListReleases() | ||
| if lerr != nil { | ||
| if errors.Is(lerr, clients.ErrUnsupportedFeature) { | ||
| return results, nil | ||
| } | ||
| return results, fmt.Errorf("RepoClient.ListReleases: %w", lerr) | ||
| } |
There was a problem hiding this comment.
similarly, this would prevent looking for SBOMs in source.
| func SignedReleases(c *checker.CheckRequest) (checker.SignedReleasesData, error) { | ||
| releases, err := c.RepoClient.ListReleases() | ||
| if err != nil { | ||
| if errors.Is(err, clients.ErrUnsupportedFeature) { | ||
| return checker.SignedReleasesData{}, nil | ||
| } |
There was a problem hiding this comment.
I'm not sure we should change this one. the Signed-Releases check relies on this data alone.
|
This pull request has been marked stale because it has been open for 10 days with no activity |
What kind of change does this PR introduce?
This check allows clients which currently do not support
ListReleasesto complete a Scorecard run. Currentlyazuredevopsrepo,localdir, andgitdo not supportListReleases. While I am working on support for Azure DevOps, I suspect thatlocaldirandgitmay never supportListReleases.What is the current behavior?
Currently, if a client does not support
ListReleases, the Scorecard run fails with an error like:This is the case for
Branch-ProtectionandSigned-Releases.What is the new behavior (if this is a feature change)?**
Which issue(s) this PR fixes
NONE
Special notes for your reviewer
This is similar to the fix done in #4423
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.)