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.

Sign up for GitHub

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

Jump to bottom

feat: Update error message #660

Closed
wants to merge 1 commit into from
Closed

feat: Update error message #660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions verifiers/internal/gha/builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ func isTrustedDelegatorBuilder(certBuilder *utils.TrustedBuilderID, trustedBuild
}

// Only allow `@refs/heads/main` for the builder and the e2e tests that need to work at HEAD.
// This includes the delegator workflows referenced by the TRWs.
// This lets us use the pre-build builder binary generated during release (release happen at main).
// For other projects, we only allow semantic versions that map to a release.
func verifyTrustedBuilderRef(id *WorkflowIdentity, ref string) error {
Expand Down
8 changes: 8 additions & 0 deletions verifiers/internal/gha/provenance.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,10 @@ func VerifyTag(prov iface.Provenance, expectedTag string) error {
}

tag, err := utils.TagFromGitRef(ref)
if tag == "" {
ianlewis marked this conversation as resolved.
Show resolved Hide resolved
return fmt.Errorf("verifying tag: %w: no tag found in provenance", serrors.ErrorMismatchTag)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("verifying tag: %w: no tag found in provenance", serrors.ErrorMismatchTag)
return fmt.Errorf("verifying tag: %w: empty tag name in provenance", serrors.ErrorMismatchTag)

}

if err != nil {
return fmt.Errorf("verifying tag: %w", err)
}
Expand All @@ -419,6 +423,10 @@ func VerifyVersionedTag(prov iface.Provenance, expectedTag string) error {
return err
}

if ref == "" {
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor Author

@laurentsimon laurentsimon Jul 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

i ran into an issue where the error message just gave empty results, because ref is empty. That happened when a user passes --source-versioned-tag but the build happened not on a release trigger.

Copy link
Member

@ianlewis ianlewis Jul 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, I think you can move this check to utils.ValidateGitRef and provide a better error message there.

Or you can do like you did above and check the tag returned by TagFromGitRef since I think this code will fail if ref is just the refs/tags/ prefix without an actual tag name.

return fmt.Errorf("verifying tag: %w: no tag found in provenance", serrors.ErrorMismatchVersionedTag)
}

tag, err := utils.TagFromGitRef(ref)
if err != nil {
return fmt.Errorf("verifying tag: %w", err)
Expand Down
6 changes: 5 additions & 1 deletion verifiers/utils/builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,11 @@ func IsValidBuilderTag(ref string, testing bool) error {
if testing {
// Tags on trusted repositories should be a valid semver with version
// core including all three parts and no build identifier.
versionCore := strings.Split(pin, "-")[0]
parts := strings.Split(pin, "-")
if len(parts) == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

nit: I'm pretty sure strings.Split will never return a slice with a length of zero (if sep is non-empty, which it is in this case). Was this an issue you actually encountered?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just read the doc and you're right. Will revert.

return fmt.Errorf("%w: %s: version tag not valid", serrors.ErrorInvalidRef, pin)
}
versionCore := parts[0]
if !semver.IsValid(pin) ||
len(strings.Split(versionCore, ".")) != 3 ||
semver.Build(pin) != "" {
Expand Down
Loading