Skip to content

Commit

Permalink
Merge pull request #1529 from octo/fix-conditions-usage
Browse files Browse the repository at this point in the history 
Fix incorrect use of format strings with the `conditions` package.
  • Loading branch information
@stefanprodan
stefanprodan authored Jul 5, 2024
2 parents 3c0dda4 + 277e5c1 commit 8d8e7cc
Show file tree
Hide file tree
Showing 6 changed files with 93 additions and 93 deletions.
32 changes: 16 additions & 16 deletions internal/controller/bucket_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ func (r *BucketReconciler) reconcile(ctx context.Context, sp *patch.SerialPatche
fmt.Errorf("failed to create temporary working directory: %w", err),
sourcev1.DirCreationFailedReason,
)
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error())
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, "%s", e)
return sreconcile.ResultEmpty, e
}
defer func() {
Expand Down Expand Up @@ -427,7 +427,7 @@ func (r *BucketReconciler) reconcileSource(ctx context.Context, sp *patch.Serial
secret, err := r.getSecret(ctx, obj.Spec.SecretRef, obj.GetNamespace())
if err != nil {
e := serror.NewGeneric(err, sourcev1.AuthenticationFailedReason)
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Error())
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e)
// Return error as the world as observed may change
return sreconcile.ResultEmpty, e
}
Expand All @@ -438,35 +438,35 @@ func (r *BucketReconciler) reconcileSource(ctx context.Context, sp *patch.Serial
case bucketv1.GoogleBucketProvider:
if err = gcp.ValidateSecret(secret); err != nil {
e := serror.NewGeneric(err, sourcev1.AuthenticationFailedReason)
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Error())
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e)
return sreconcile.ResultEmpty, e
}
if provider, err = gcp.NewClient(ctx, secret); err != nil {
e := serror.NewGeneric(err, "ClientError")
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Error())
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e)
return sreconcile.ResultEmpty, e
}
case bucketv1.AzureBucketProvider:
if err = azure.ValidateSecret(secret); err != nil {
e := serror.NewGeneric(err, sourcev1.AuthenticationFailedReason)
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Error())
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e)
return sreconcile.ResultEmpty, e
}
if provider, err = azure.NewClient(obj, secret); err != nil {
e := serror.NewGeneric(err, "ClientError")
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Error())
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e)
return sreconcile.ResultEmpty, e
}
default:
if err = minio.ValidateSecret(secret); err != nil {
e := serror.NewGeneric(err, sourcev1.AuthenticationFailedReason)
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Error())
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e)
return sreconcile.ResultEmpty, e
}
tlsConfig, err := r.getTLSConfig(ctx, obj)
if err != nil {
e := serror.NewGeneric(err, sourcev1.AuthenticationFailedReason)
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Error())
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e)
return sreconcile.ResultEmpty, e
}
proxyURL, err := r.getProxyURL(ctx, obj)
Expand All @@ -487,15 +487,15 @@ func (r *BucketReconciler) reconcileSource(ctx context.Context, sp *patch.Serial
}
if provider, err = minio.NewClient(obj, opts...); err != nil {
e := serror.NewGeneric(err, "ClientError")
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Error())
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e)
return sreconcile.ResultEmpty, e
}
}

// Fetch etag index
if err = fetchEtagIndex(ctx, provider, obj, index, dir); err != nil {
e := serror.NewGeneric(err, bucketv1.BucketOperationFailedReason)
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Error())
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e)
return sreconcile.ResultEmpty, e
}

Expand All @@ -516,7 +516,7 @@ func (r *BucketReconciler) reconcileSource(ctx context.Context, sp *patch.Serial

message := fmt.Sprintf("new upstream revision '%s'", revision)
if obj.GetArtifact() != nil {
conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", message)
conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", "%s", message)
}
rreconcile.ProgressiveStatus(true, obj, meta.ProgressingReason, "building artifact: %s", message)
if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil {
Expand All @@ -527,7 +527,7 @@ func (r *BucketReconciler) reconcileSource(ctx context.Context, sp *patch.Serial

if err = fetchIndexFiles(ctx, provider, obj, index, dir); err != nil {
e := serror.NewGeneric(err, bucketv1.BucketOperationFailedReason)
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Error())
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e)
return sreconcile.ResultEmpty, e
}
}
Expand Down Expand Up @@ -579,14 +579,14 @@ func (r *BucketReconciler) reconcileArtifact(ctx context.Context, sp *patch.Seri
fmt.Errorf("failed to stat source path: %w", err),
sourcev1.StatOperationFailedReason,
)
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error())
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, "%s", e)
return sreconcile.ResultEmpty, e
} else if !f.IsDir() {
e := serror.NewGeneric(
fmt.Errorf("source path '%s' is not a directory", dir),
sourcev1.InvalidPathReason,
)
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error())
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, "%s", e)
return sreconcile.ResultEmpty, e
}

Expand All @@ -596,7 +596,7 @@ func (r *BucketReconciler) reconcileArtifact(ctx context.Context, sp *patch.Seri
fmt.Errorf("failed to create artifact directory: %w", err),
sourcev1.DirCreationFailedReason,
)
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error())
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, "%s", e)
return sreconcile.ResultEmpty, e
}
unlock, err := r.Storage.Lock(artifact)
Expand All @@ -614,7 +614,7 @@ func (r *BucketReconciler) reconcileArtifact(ctx context.Context, sp *patch.Seri
fmt.Errorf("unable to archive artifact to storage: %s", err),
sourcev1.ArchiveOperationFailedReason,
)
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error())
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, "%s", e)
return sreconcile.ResultEmpty, e
}

Expand Down
46 changes: 23 additions & 23 deletions internal/controller/gitrepository_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ func (r *GitRepositoryReconciler) reconcile(ctx context.Context, sp *patch.Seria
fmt.Errorf("failed to create temporary working directory: %w", err),
sourcev1.DirCreationFailedReason,
)
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error())
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, "%s", e)
return sreconcile.ResultEmpty, e
}
defer func() {
Expand Down Expand Up @@ -486,7 +486,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, sp *patch
fmt.Errorf("failed to configure proxy options: %w", err),
sourcev1.AuthenticationFailedReason,
)
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e)
// Return error as the world as observed may change
return sreconcile.ResultEmpty, e
}
Expand All @@ -498,7 +498,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, sp *patch
fmt.Errorf("failed to parse url '%s': %w", obj.Spec.URL, err),
sourcev1.URLInvalidReason,
)
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e)
return sreconcile.ResultEmpty, e
}

Expand All @@ -508,7 +508,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, sp *patch
fmt.Errorf("failed to configure authentication options: %w", err),
sourcev1.AuthenticationFailedReason,
)
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e)
// Return error as the world as observed may change
return sreconcile.ResultEmpty, e
}
Expand All @@ -523,7 +523,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, sp *patch
if artifacts.Diff(obj.Status.IncludedArtifacts) {
message := "included artifacts differ from last observed includes"
if obj.Status.IncludedArtifacts != nil {
conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "IncludeChange", message)
conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "IncludeChange", "%s", message)
}
rreconcile.ProgressiveStatus(true, obj, meta.ProgressingReason, "building artifact: %s", message)
if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil {
Expand All @@ -544,7 +544,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, sp *patch
fmt.Errorf("git repository is empty"),
"EmptyGitRepository",
)
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e)
return sreconcile.ResultEmpty, e
}
// Assign the commit to the shared commit reference.
Expand Down Expand Up @@ -597,7 +597,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, sp *patch
if !obj.GetArtifact().HasRevision(commitReference(obj, commit)) {
message := fmt.Sprintf("new upstream revision '%s'", commitReference(obj, commit))
if obj.GetArtifact() != nil {
conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", message)
conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", "%s", message)
}
rreconcile.ProgressiveStatus(true, obj, meta.ProgressingReason, "building artifact: %s", message)
if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil {
Expand Down Expand Up @@ -703,14 +703,14 @@ func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context, sp *pat
fmt.Errorf("failed to stat target artifact path: %w", err),
sourcev1.StatOperationFailedReason,
)
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error())
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, "%s", e)
return sreconcile.ResultEmpty, e
} else if !f.IsDir() {
e := serror.NewGeneric(
fmt.Errorf("invalid target path: '%s' is not a directory", dir),
sourcev1.InvalidPathReason,
)
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error())
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, "%s", e)
return sreconcile.ResultEmpty, e
}

Expand All @@ -720,7 +720,7 @@ func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context, sp *pat
fmt.Errorf("failed to create artifact directory: %w", err),
sourcev1.DirCreationFailedReason,
)
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error())
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, "%s", e)
return sreconcile.ResultEmpty, e
}
unlock, err := r.Storage.Lock(artifact)
Expand Down Expand Up @@ -751,7 +751,7 @@ func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context, sp *pat
fmt.Errorf("unable to archive artifact to storage: %w", err),
sourcev1.ArchiveOperationFailedReason,
)
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error())
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, "%s", e)
return sreconcile.ResultEmpty, e
}

Expand Down Expand Up @@ -800,7 +800,7 @@ func (r *GitRepositoryReconciler) reconcileInclude(ctx context.Context, sp *patc
fmt.Errorf("path calculation for include '%s' failed: %w", incl.GitRepositoryRef.Name, err),
"IllegalPath",
)
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error())
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, "%s", e)
return sreconcile.ResultEmpty, e
}

Expand All @@ -821,7 +821,7 @@ func (r *GitRepositoryReconciler) reconcileInclude(ctx context.Context, sp *patc
fmt.Errorf("failed to copy '%s' include from %s to %s: %w", incl.GitRepositoryRef.Name, incl.GetFromPath(), incl.GetToPath(), err),
"CopyFailure",
)
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, e.Err.Error())
conditions.MarkTrue(obj, sourcev1.StorageOperationFailedCondition, e.Reason, "%s", e)
return sreconcile.ResultEmpty, e
}
}
Expand Down Expand Up @@ -872,7 +872,7 @@ func (r *GitRepositoryReconciler) gitCheckout(ctx context.Context, obj *sourcev1
fmt.Errorf("failed to create Git client: %w", err),
sourcev1.GitOperationFailedReason,
)
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e)
return nil, e
}
defer gitReader.Close()
Expand All @@ -883,7 +883,7 @@ func (r *GitRepositoryReconciler) gitCheckout(ctx context.Context, obj *sourcev1
fmt.Errorf("failed to checkout and determine revision: %w", err),
sourcev1.GitOperationFailedReason,
)
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, "%s", e)
return nil, e
}

Expand All @@ -902,7 +902,7 @@ func (r *GitRepositoryReconciler) fetchIncludes(ctx context.Context, obj *source
"NotFound",
)
e.RequeueAfter = r.requeueDependency
conditions.MarkTrue(obj, sourcev1.IncludeUnavailableCondition, e.Reason, e.Err.Error())
conditions.MarkTrue(obj, sourcev1.IncludeUnavailableCondition, e.Reason, "%s", e)
return nil, e
}

Expand All @@ -913,7 +913,7 @@ func (r *GitRepositoryReconciler) fetchIncludes(ctx context.Context, obj *source
"NoArtifact",
)
e.RequeueAfter = r.requeueDependency
conditions.MarkTrue(obj, sourcev1.IncludeUnavailableCondition, e.Reason, e.Err.Error())
conditions.MarkTrue(obj, sourcev1.IncludeUnavailableCondition, e.Reason, "%s", e)
return nil, e
}

Expand Down Expand Up @@ -953,7 +953,7 @@ func (r *GitRepositoryReconciler) verifySignature(ctx context.Context, obj *sour
fmt.Errorf("PGP public keys secret error: %w", err),
"VerificationError",
)
conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, e.Reason, e.Err.Error())
conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, e.Reason, "%s", e)
return sreconcile.ResultEmpty, e
}

Expand All @@ -974,7 +974,7 @@ func (r *GitRepositoryReconciler) verifySignature(ctx context.Context, obj *sour
errors.New("cannot verify tag object's signature if a tag reference is not specified"),
"InvalidVerificationMode",
)
conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, err.Reason, err.Err.Error())
conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, err.Reason, "%s", err)
return sreconcile.ResultEmpty, err
}
if !git.IsSignedTag(*tag) {
Expand All @@ -985,7 +985,7 @@ func (r *GitRepositoryReconciler) verifySignature(ctx context.Context, obj *sour
fmt.Errorf("cannot verify signature of tag '%s' since it is not signed", commit.ReferencingTag.String()),
"InvalidGitObject",
)
conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, err.Reason, err.Err.Error())
conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, err.Reason, "%s", err)
return sreconcile.ResultEmpty, err
}

Expand All @@ -996,7 +996,7 @@ func (r *GitRepositoryReconciler) verifySignature(ctx context.Context, obj *sour
fmt.Errorf("signature verification of tag '%s' failed: %w", tag.String(), err),
"InvalidTagSignature",
)
conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, e.Reason, e.Err.Error())
conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, e.Reason, "%s", e)
// Return error in the hope the secret changes
return sreconcile.ResultEmpty, e
}
Expand All @@ -1012,7 +1012,7 @@ func (r *GitRepositoryReconciler) verifySignature(ctx context.Context, obj *sour
fmt.Errorf("signature verification of commit '%s' failed: %w", commit.Hash.String(), err),
"InvalidCommitSignature",
)
conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, e.Reason, e.Err.Error())
conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, e.Reason, "%s", e)
// Return error in the hope the secret changes
return sreconcile.ResultEmpty, e
}
Expand All @@ -1027,7 +1027,7 @@ func (r *GitRepositoryReconciler) verifySignature(ctx context.Context, obj *sour
reason := meta.SucceededReason
mode := obj.Spec.Verification.GetMode()
obj.Status.SourceVerificationMode = &mode
conditions.MarkTrue(obj, sourcev1.SourceVerifiedCondition, reason, message.String())
conditions.MarkTrue(obj, sourcev1.SourceVerifiedCondition, reason, "%s", message.String())
r.eventLogf(ctx, obj, eventv1.EventTypeTrace, reason, message.String())
return sreconcile.ResultSuccess, nil
}
Expand Down
Loading

0 comments on commit 8d8e7cc

Please sign in to comment.