Skip to content

Migrate to Structured Logging with zerolog and Improve Logging for Manifest Purge Logic #473

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

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Migrate to Structured Logging with zerolog and Improve Logging for Manifest Purge Logic #473

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8910185
Initial plan
Copilot Jul 9, 2025
f1a3997
Initial plan for structured logging migration
Copilot Jul 9, 2025
009bb50
Add zerolog dependency and implement structured logging for manifest …
Copilot Jul 9, 2025
6fcf423
Complete structured logging migration for annotate, cssc, and manifes…
Copilot Jul 9, 2025
12c3f19
Complete structured logging implementation with comprehensive documen…
Copilot Jul 9, 2025
42e878c
Address PR review feedback: logger constants, remove fmt.Printf, opti…
Copilot Jul 9, 2025
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
3 changes: 2 additions & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,5 @@
coverage.txt
*.test
*.out
*.html
*.html
acr-bin
30 changes: 30 additions & 0 deletions cmd/acr/annotate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (

"github.com/Azure/acr-cli/cmd/common"
"github.com/Azure/acr-cli/internal/api"
"github.com/Azure/acr-cli/internal/logger"
"github.com/Azure/acr-cli/internal/worker"
"github.com/dlclark/regexp2"
"github.com/spf13/cobra"
Expand Down Expand Up @@ -163,8 +164,17 @@ func annotateTags(ctx context.Context,
dryRun bool) (int, error) {

if !dryRun {
log := logger.Get()
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like this constant logger.Get() . The logger should be acquired once at the top of the function and then used as required.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed logger.Get() usage pattern - now acquiring logger once at function top instead of repeated calls. Changes in commit 42e878c.

log.Info().
Str("repository", repoName).
Msg("Starting tag annotation for repository")
fmt.Printf("\nAnnotating tags for repository: %s\n", repoName)
} else {
log := logger.Get()
log.Info().
Str("repository", repoName).
Bool("dry_run", true).
Msg("Dry run: would annotate tags for repository")
fmt.Printf("\nTags for this repository would be annotated: %s\n", repoName)
}

Expand Down Expand Up @@ -225,6 +235,11 @@ func getManifestsToAnnotate(ctx context.Context,
resultTags, err := acrClient.GetAcrTags(ctx, repoName, "timedesc", lastTag)
if err != nil {
if resultTags != nil && resultTags.Response.Response != nil && resultTags.StatusCode == http.StatusNotFound {
log := logger.Get()
log.Warn().
Str("repository", repoName).
Int("status_code", resultTags.StatusCode).
Msg("Repository not found during annotation operation")
fmt.Printf("%s repository not found\n", repoName)
Copy link
Contributor

Choose a reason for hiding this comment

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

all fmt.Printf messages should be removed as they are being replaced by the new structured logger

Copy link
Author

Choose a reason for hiding this comment

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

Removed fmt.Printf statements from annotate.go as requested. Replaced with structured logging where appropriate. Changes in commit 42e878c.

return nil, "", nil
}
Expand Down Expand Up @@ -257,6 +272,12 @@ func getManifestsToAnnotate(ctx context.Context,
// Only print what would be annotated during a dry-run. Successfully annotated manifests
// will be logged after the annotation.
if dryRun {
log := logger.Get()
log.Debug().
Str("repository", repoName).
Str("tag", *tag.Name).
Str("manifest", *tag.Digest).
Msg("Tag marked for annotation in dry run")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should use Bool("dry_run", true)

Copy link
Author

Choose a reason for hiding this comment

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

Fixed to use Bool("dry_run", true) instead of string format. Changes in commit 42e878c.

fmt.Printf("%s/%s:%s\n", loginURL, repoName, *tag.Name)
}
manifestsToAnnotate = append(manifestsToAnnotate, *tag.Digest)
Expand All @@ -280,8 +301,17 @@ func annotateUntaggedManifests(ctx context.Context,
annotations []string,
dryRun bool) (int, error) {
if !dryRun {
log := logger.Get()
log.Info().
Str("repository", repoName).
Msg("Starting manifest annotation for repository")
fmt.Printf("Annotating manifests for repository: %s\n", repoName)
} else {
log := logger.Get()
log.Info().
Str("repository", repoName).
Bool("dry_run", true).
Msg("Dry run: would annotate manifests for repository")
fmt.Printf("Manifests for this repository would be annotated: %s\n", repoName)
}

Expand Down
56 changes: 56 additions & 0 deletions cmd/acr/manifest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"fmt"

"github.com/Azure/acr-cli/internal/api"
"github.com/Azure/acr-cli/internal/logger"
"github.com/pkg/errors"
"github.com/spf13/cobra"
)
Expand Down Expand Up @@ -83,29 +84,56 @@ func newManifestListCmd(manifestParams *manifestParameters) *cobra.Command {

// listManifests will do the http requests and print the digest of all the manifest in the selected repository.
func listManifests(ctx context.Context, acrClient api.AcrCLIClientInterface, loginURL string, repoName string) error {
log := logger.Get()

log.Debug().
Str("repository", repoName).
Copy link
Contributor

Choose a reason for hiding this comment

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

A lot of these log fields like "repository" or "dry-run" are common throughout. Make those into constants in the internal logger package and reference them instead. Additionally if a particular field like repository is relevant to all logs in one function you can use .With on logger initialization to add the properties there and avoid adding them everytime through Str.

Copy link
Author

Choose a reason for hiding this comment

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

Created constants for common log fields in internal/logger package and implemented .With() pattern for repository context to avoid repetition. Changes in commit 42e878c.

Msg("Starting manifest listing")

lastManifestDigest := ""
resultManifests, err := acrClient.GetAcrManifests(ctx, repoName, "", lastManifestDigest)
if err != nil {
log.Error().
Err(err).
Str("repository", repoName).
Msg("Failed to get manifests for listing")
return errors.Wrap(err, "failed to list manifests")
}

fmt.Printf("Listing manifests for the %q repository:\n", repoName)

totalManifests := 0
// A for loop is used because the GetAcrManifests method returns by default only 100 manifests and their attributes.
for resultManifests != nil && resultManifests.ManifestsAttributes != nil {
manifests := *resultManifests.ManifestsAttributes
for _, manifest := range manifests {
manifestDigest := *manifest.Digest
fmt.Printf("%s/%s@%s\n", loginURL, repoName, manifestDigest)

log.Debug().
Str("repository", repoName).
Str("manifest", manifestDigest).
Msg("Listed manifest")
totalManifests++
}
// Since the GetAcrManifests supports pagination when supplied with the last digest that was returned the last manifest
// digest is saved, the manifest array contains at least one element because if it was empty the API would return
// a nil pointer instead of a pointer to a length 0 array.
lastManifestDigest = *manifests[len(manifests)-1].Digest
resultManifests, err = acrClient.GetAcrManifests(ctx, repoName, "", lastManifestDigest)
if err != nil {
log.Error().
Copy link
Contributor

Choose a reason for hiding this comment

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

Errors are propagated through the errors.Wrap. There is no no need to log the error at every level. Additionally, where seen errors.wrap should be replaced with fmt.Errorf(%w).

Copy link
Author

Choose a reason for hiding this comment

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

Replaced errors.Wrap with fmt.Errorf(%w) and removed redundant error logging at every level. Changes in commit 42e878c.

Err(err).
Str("repository", repoName).
Msg("Failed to get next page of manifests")
return errors.Wrap(err, "failed to list manifests")
}
}

log.Info().
Str("repository", repoName).
Int("total_manifests", totalManifests).
Msg("Completed manifest listing")
return nil
}

Expand Down Expand Up @@ -140,13 +168,41 @@ func newManifestDeleteCmd(manifestParams *manifestParameters) *cobra.Command {

// deleteManifests receives an array of manifests digest and deletes them using the supplied acrClient.
func deleteManifests(ctx context.Context, acrClient api.AcrCLIClientInterface, loginURL string, repoName string, args []string) error {
log := logger.Get()

log.Info().
Str("repository", repoName).
Int("manifest_count", len(args)).
Msg("Starting manifest deletion")

for i := 0; i < len(args); i++ {
_, err := acrClient.DeleteManifest(ctx, repoName, args[i])
if err != nil {
log.Error().
Err(err).
Str("repository", repoName).
Str("manifest", args[i]).
Int("position", i+1).
Int("total", len(args)).
Msg("Failed to delete manifest")
// If there is an error (this includes not found and not allowed operations) the deletion of the images is stopped and an error is returned.
return errors.Wrap(err, "failed to delete manifests")
}

fmt.Printf("%s/%s@%s\n", loginURL, repoName, args[i])

log.Info().
Str("repository", repoName).
Str("manifest", args[i]).
Str("ref", fmt.Sprintf("%s/%s@%s", loginURL, repoName, args[i])).
Int("position", i+1).
Int("total", len(args)).
Msg("Successfully deleted manifest")
}

log.Info().
Str("repository", repoName).
Int("deleted_count", len(args)).
Msg("Completed manifest deletion")
return nil
}
49 changes: 48 additions & 1 deletion cmd/acr/purge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"github.com/Azure/acr-cli/acr"
"github.com/Azure/acr-cli/cmd/common"
"github.com/Azure/acr-cli/internal/api"
"github.com/Azure/acr-cli/internal/logger"
"github.com/Azure/acr-cli/internal/worker"
"github.com/dlclark/regexp2"
"github.com/spf13/cobra"
Expand Down Expand Up @@ -348,29 +349,75 @@ func getTagsToDelete(ctx context.Context,
// purgeDanglingManifests deletes all manifests that do not have any tags associated with them.
// except the ones that are referenced by a multiarch manifest or that have subject.
func purgeDanglingManifests(ctx context.Context, acrClient api.AcrCLIClientInterface, repoParallelism int, loginURL string, repoName string, manifestToTagsCountMap map[string]int, dryRun bool) (int, error) {
fmt.Printf("Deleting manifests for repository: %s\n", repoName)
log := logger.Get()

log.Info().
Str("repository", repoName).
Bool("dry_run", dryRun).
Msg("Starting manifest purge operation")

// Contrary to getTagsToDelete, getManifestsToDelete gets all the Manifests at once, this was done because if there is a manifest that has no
// tag but is referenced by a multiarch manifest that has tags then it should not be deleted. Or if a manifest has no tag, but it has subject,
// then it should not be deleted.
manifestsToDelete, err := common.GetUntaggedManifests(ctx, repoParallelism, acrClient, repoName, false, manifestToTagsCountMap, dryRun)
if err != nil {
log.Error().
Err(err).
Str("repository", repoName).
Msg("Failed to get untagged manifests")
return -1, err
}

log.Info().
Str("repository", repoName).
Int("candidate_count", len(manifestsToDelete)).
Msg("Found candidate manifests for deletion")

// If dryRun is set to true then no manifests will be deleted, but the number of manifests that would be deleted is returned. Additionally,
// the manifests that would be deleted are printed to the console. We also need to account for the manifests that would be deleted from the tag
// filtering first as that would influence the untagged manifests that would be deleted.
if dryRun {
log.Info().
Str("repository", repoName).
Int("count", len(manifestsToDelete)).
Msg("Dry run: manifests that would be deleted")

for _, manifest := range manifestsToDelete {
// Keep the fmt.Printf for user output consistency
fmt.Printf("Would delete: %s/%s@%s\n", loginURL, repoName, manifest)

log.Debug().
Str("repository", repoName).
Str("manifest", manifest).
Str("ref", fmt.Sprintf("%s/%s@%s", loginURL, repoName, manifest)).
Msg("Manifest marked for deletion in dry run")
}
return len(manifestsToDelete), nil
}

// In order to only have a limited amount of http requests, a purger is used that will start goroutines to delete manifests.
log.Debug().
Str("repository", repoName).
Int("parallelism", repoParallelism).
Int("manifest_count", len(manifestsToDelete)).
Msg("Starting concurrent manifest deletion")

purger := worker.NewPurger(repoParallelism, acrClient, loginURL, repoName)
deletedManifestsCount, purgeErr := purger.PurgeManifests(ctx, manifestsToDelete)
if purgeErr != nil {
log.Error().
Err(purgeErr).
Str("repository", repoName).
Int("attempted_count", len(manifestsToDelete)).
Msg("Failed to purge manifests")
return -1, purgeErr
}

log.Info().
Str("repository", repoName).
Int("deleted_count", deletedManifestsCount).
Int("attempted_count", len(manifestsToDelete)).
Msg("Successfully completed manifest purge operation")

return deletedManifestsCount, nil
}
12 changes: 12 additions & 0 deletions cmd/acr/root.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"errors"
"os"

"github.com/Azure/acr-cli/internal/logger"
"github.com/spf13/cobra"
)

Expand All @@ -16,6 +17,8 @@ type rootParameters struct {
username string
password string
configs []string
logLevel string
logFormat string
}

func newRootCmd(args []string) *cobra.Command {
Expand All @@ -32,6 +35,13 @@ func newRootCmd(args []string) *cobra.Command {

To start working with the CLI, run acr --help`,
SilenceUsage: true,
PersistentPreRun: func(cmd *cobra.Command, args []string) {
// Setup logger with configured level and format
logger.Setup(logger.Config{
Level: rootParams.logLevel,
Format: rootParams.logFormat,
})
},
}

flags := cmd.PersistentFlags()
Expand All @@ -55,6 +65,8 @@ To start working with the CLI, run acr --help`,
cmd.PersistentFlags().StringVarP(&rootParams.registryName, "registry", "r", "", "Registry name")
cmd.PersistentFlags().StringVarP(&rootParams.username, "username", "u", "", "Registry username")
cmd.PersistentFlags().StringVarP(&rootParams.password, "password", "p", "", "Registry password")
cmd.PersistentFlags().StringVar(&rootParams.logLevel, "log-level", "info", "Log level (debug, info, warn, error)")
cmd.PersistentFlags().StringVar(&rootParams.logFormat, "log-format", "console", "Log format (console, json)")
cmd.Flags().BoolP("help", "h", false, "Print usage")
cmd.Flags().StringArrayVarP(&rootParams.configs, "config", "c", nil, "Auth config paths")
// No parameter is marked as required because the registry could be inferred from a task context, same with username and password
Expand Down
Loading