{{ message }}
Update verification results printing#9937
Merged
Merged
Conversation
Signed-off-by: Meredith Lancaster <malancas@github.com>
Signed-off-by: Meredith Lancaster <malancas@github.com>
phillmv
reviewed
Nov 20, 2024
phillmv
left a comment
phillmv
left a comment
Contributor
There was a problem hiding this comment.
I still need to read over the tests but here is a prelim comment
Signed-off-by: Meredith Lancaster <malancas@github.com>
Signed-off-by: Meredith Lancaster <malancas@github.com>
Signed-off-by: Meredith Lancaster <malancas@github.com>
Signed-off-by: Meredith Lancaster <malancas@github.com>
Signed-off-by: Meredith Lancaster <malancas@github.com>
Signed-off-by: Meredith Lancaster <malancas@github.com>
Signed-off-by: Meredith Lancaster <malancas@github.com>
phillmv
reviewed
Nov 21, 2024
phillmv
left a comment
phillmv
left a comment
Contributor
There was a problem hiding this comment.
I approve of the shape of this PR and I have only not clicked Approve because I think we should improve some comments (see my feedback)
Contributor
There was a problem hiding this comment.
ideally i'd like to check that the rwfResult is specifically the one being excluded. can we do an array comparison for sgjAttestation[0] and sgjAttestation[1]?
| return &MockSigstoreVerifier{t, mockResults} | ||
| } | ||
|
|
||
| func NewDefaultMockSigstoreVerifier(t *testing.T) *MockSigstoreVerifier { |
Contributor
There was a problem hiding this comment.
food for thought / not high priority suggestion: considering the nature of this change i'd invert this: NewMockSigstoreVerifierWithMockResults so we don't modify every other usage of it / i feel like it's harder to reason what the Default Mock is.
Contributor
Author
There was a problem hiding this comment.
Agreed, we can definitely iterate on this one
| // if at least one attestation is verified, we're good as verification | ||
| // is defined as successful if at least one attestation is verified | ||
| return nil | ||
| if err := verifyCertExtensions(*attestation.VerificationResult.Signature.Certificate, ec.Certificate); err != nil { |
Contributor
There was a problem hiding this comment.
yesterday we had a conversation where we realized it's not obvious WHY we verifyCertExtensions separately from the CertificateIdentity provided in sigstore-go (and frankly, maybe we should just upstream how we've done it here) and
after some mild effort,
we discovered it's so we can support case insensitivity around repo & owner names.
given that conversation let's add a wee comment to func verifyCertExtensiosn denoting that - "this func exists so we can do case insensitive comparisons"
Co-authored-by: Phill MV <phillmv@github.com>
Signed-off-by: Meredith Lancaster <malancas@github.com>
cliAutomation
added
the
external
Contributor
Signed-off-by: Meredith Lancaster <malancas@github.com>
tmeijn
pushed a commit
to tmeijn/dotfiles
that referenced
this pull request
Nov 28, 2024
This MR contains the following updates: | Package | Update | Change | |---|---|---| | [cli/cli](https://github.com/cli/cli) | minor | `v2.62.0` -> `v2.63.0` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>cli/cli (cli/cli)</summary> ### [`v2.63.0`](https://github.com/cli/cli/releases/tag/v2.63.0): GitHub CLI 2.63.0 [Compare Source](cli/cli@v2.62.0...v2.63.0) #### What's Changed - Support bare repo creation by [@​williammartin](https://github.com/williammartin) in cli/cli#9905 - Refactor the `getAttestations` functions by [@​malancas](https://github.com/malancas) in cli/cli#9892 - Added a section on manual verification of the relases. by [@​kommendorkapten](https://github.com/kommendorkapten) in cli/cli#9936 - Adding option to return `baseRefOid` in `pr view` by [@​daliusd](https://github.com/daliusd) in cli/cli#9938 - Update verification results printing by [@​malancas](https://github.com/malancas) in cli/cli#9937 - Fix some multiline command documentation to use `heredoc` strings by [@​BagToad](https://github.com/BagToad) in cli/cli#9948 - Print friendly error when `release create` fails due to missing `workflow` OAuth scope by [@​BagToad](https://github.com/BagToad) in cli/cli#9791 **Full Changelog**: cli/cli@v2.62.0...v2.63.0 #### Security - A security vulnerability has been identified in the GitHub CLI that could leak authentication tokens when cloning repositories containing git submodules hosted outside of GitHub.com and ghe.com. For more information, see GHSA-jwcm-9g39-pmcw #### New Contributors - [@​daliusd](https://github.com/daliusd) made their first contribution in cli/cli#9938 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40NDAuNyIsInVwZGF0ZWRJblZlciI6IjM3LjQ0MC43IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Improve how we filter attestations shown as verification results to the user cc #9850