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

🐛 Fix signed release error for empty gitlab repo #3753

Merged
merged 3 commits into from
Dec 30, 2023

Conversation

naveensrinivasan
Copy link
Member

What kind of change does this PR introduce?

  • Fixed the issue where an empty gitlab repo is causing this error. Error: check runtime error: Signed-Releases: internal error: could not get release name 2023/12/27 18:07:19 error during command execution: check runtime error: Signed-Releases: internal error: could not get release name exit status 1

(Is it a bug fix, feature, docs update, something else?)

What is the current behavior?

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

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

Which issue(s) this PR fixes

Special notes for your reviewer

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


@naveensrinivasan naveensrinivasan self-assigned this Dec 28, 2023
@naveensrinivasan naveensrinivasan requested a review from a team as a code owner December 28, 2023 15:22
@naveensrinivasan naveensrinivasan requested review from spencerschrock and raghavkaul and removed request for a team December 28, 2023 15:22
@naveensrinivasan naveensrinivasan requested review from a team and laurentsimon and removed request for raghavkaul, a team and laurentsimon December 28, 2023 15:22
Copy link

codecov bot commented Dec 28, 2023

Codecov Report

Merging #3753 (d4be15a) into main (a34f0bf) will increase coverage by 4.07%.
Report is 1 commits behind head on main.
The diff coverage is 57.14%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3753      +/-   ##
==========================================
+ Coverage   66.83%   70.90%   +4.07%     
==========================================
  Files         227      227              
  Lines       15356    15353       -3     
==========================================
+ Hits        10263    10886     +623     
+ Misses       4486     3802     -684     
- Partials      607      665      +58     

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.

As part of our structured result works, we try to use the finding outcomes when determining things like "no releases".

In this case, the convention is a single finding with OutcomeNotApplicable.

short: Check that the projects Github and Gitlab releases are signed.
motivation: >
Signed releases allow consumers to verify their artifacts before consuming them.
implementation: >
The implementation checks whether a signature file is present in release assets. The probe checks the last 5 releases on Github and Gitlab.
outcome:
- For each of the last 5 releases, the probe returns OutcomePositive, if the release has a signature file in the release assets.
- For each of the last 5 releases, the probe returns OutcomeNegative, if the the release does not have a signature file in the release assets.
- If the project has no releases, the probe returns OutcomeNotApplicable.

checks/evaluation/signed_releases.go Show resolved Hide resolved
@naveensrinivasan
Copy link
Member Author

@spencerschrock Let me know if this is good.

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 moving the if f.Outcome == finding.OutcomeNotApplicable block up is all that was needed.

I'm fine with the getReleaseName function signature changes, but still think not having a release name should be considered an error as it would indicate a programming mistake on our side, not anything wrong with the repo.

checks/evaluation/signed_releases.go Outdated Show resolved Hide resolved
checks/evaluation/signed_releases.go Outdated Show resolved Hide resolved
checks/evaluation/signed_releases.go Outdated Show resolved Hide resolved
checks/evaluation/signed_releases.go Outdated Show resolved Hide resolved
@spencerschrock
Copy link
Member

/scdiff generate Signed-Releases

(just a tool to help visualize changes across 15 test repos)

Copy link

- Fixed the issue where an empty gitlab repo is causing this error.
`Error: check runtime error: Signed-Releases: internal error: could not get release name
2023/12/27 18:07:19 error during command execution: check runtime error: Signed-Releases: internal error: could not get release name
exit status 1`

Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
@naveensrinivasan
Copy link
Member Author

I think moving the if f.Outcome == finding.OutcomeNotApplicable block up is all that was needed.

I'm fine with the getReleaseName function signature changes, but still think not having a release name should be considered an error as it would indicate a programming mistake on our side, not anything wrong with the repo.

Thanks

@naveensrinivasan naveensrinivasan enabled auto-merge (squash) December 30, 2023 16:38
@naveensrinivasan naveensrinivasan merged commit 1177c3c into main Dec 30, 2023
38 checks passed
@naveensrinivasan naveensrinivasan deleted the naveen/fix/empty-signed-release branch December 30, 2023 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants