Skip to content

Conversation

balcsida
Copy link
Contributor

  • Apply the purge command’s age threshold to manifest cleanup by deriving the cutoff in purgeDanglingManifests and filtering untagged manifests by LastUpdateTime before deletion so only older content is removed.
  • Keep annotation support aligned with the new helper signature by passing a nil cutoff when collecting untagged manifests for annotate workflows.
  • Document the age-respecting manifest purge behavior and add unit plus smoke coverage to confirm recent untagged images are preserved until they age past the cutoff.

Discovered while finding answer for #515

#### Untagged flag

To delete all the manifests that do not have any tags linked to them, the `--untagged` flag should be set.
To delete all the manifests that do not have any tags linked to them, the `--untagged` flag should be set. The manifest cleanup respects the same `--ago` cutoff that is used for tag deletions, so recently-untagged images that are newer than the configured age threshold are preserved.
Copy link
Member

@FeynmanZhou FeynmanZhou Oct 8, 2025

Choose a reason for hiding this comment

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

Can we also update the command examples for the flag --ago to tell users about how to use --ago with --untagged together to configure a fine-grained age threshold of the purge policy?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add the examples to the consts at the top of purge.go? Also updating the command info for --ago there as it currently states:

cmd.Flags().StringVar(&purgeParams.ago, "ago", "", "The tags that were last updated before this duration will be deleted, the format is [number]d[string] where the first number represents an amount of days and the string is in a Go duration format (e.g. 2d3h6m selects images older than 2 days, 3 hours and 6 minutes)")

which wouldn't reflect the new changes. I think beyond that the PR is ready.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing, I'll make this small update as soon as I can

Copy link
Member

@FeynmanZhou FeynmanZhou left a comment

Choose a reason for hiding this comment

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

Thanks @balcsida for the 4th PR! @estebanreyl will help review the code.

@FeynmanZhou FeynmanZhou requested a review from Copilot October 8, 2025 01:14
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements age-based filtering for untagged manifest purging to honor the --ago cutoff parameter. The change ensures that when using the --untagged flag with --ago, only untagged manifests older than the specified time threshold are deleted, preserving recent content.

Key changes:

  • Extended GetUntaggedManifests function to accept an optional age cutoff parameter
  • Modified purgeDanglingManifests to calculate and pass the age threshold to manifest filtering
  • Updated all call sites to maintain compatibility with the new function signature

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
cmd/repository/image_functions.go Added age filtering logic to respect --ago cutoff when collecting untagged manifests
cmd/acr/purge.go Modified to parse age duration and pass cutoff to manifest collection
cmd/acr/annotate.go Updated to pass nil cutoff for annotation workflows
cmd/acr/purge_test.go Added unit tests and updated existing tests to use new function signature
scripts/experimental/test-purge-all.sh Added smoke tests to verify age-based manifest filtering behavior
README.md Updated documentation to explain age-respecting manifest cleanup

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +320 to +324
manifestList := &acr.Manifests{
Registry: &testLoginURL,
ImageName: &testRepo,
ManifestsAttributes: &[]acr.ManifestAttributesBase{{
LastUpdateTime: &lastUpdateTime,
Copy link

Copilot AI Oct 8, 2025

Choose a reason for hiding this comment

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

The test references lastUpdateTime2DaysAgo variable but this variable is not defined in the visible test context. This will cause a compilation error.

Suggested change
manifestList := &acr.Manifests{
Registry: &testLoginURL,
ImageName: &testRepo,
ManifestsAttributes: &[]acr.ManifestAttributesBase{{
LastUpdateTime: &lastUpdateTime,
lastUpdateTime2DaysAgo := time.Now().Add(-48 * time.Hour)
manifestList := &acr.Manifests{
Registry: &testLoginURL,
ImageName: &testRepo,
ManifestsAttributes: &[]acr.ManifestAttributesBase{{
LastUpdateTime: &lastUpdateTime2DaysAgo,

Copilot uses AI. Check for mistakes.

Registry: &testLoginURL,
ImageName: &testRepo,
ManifestsAttributes: &[]acr.ManifestAttributesBase{{
LastUpdateTime: &lastUpdateTime2DaysAgo,
Copy link

Copilot AI Oct 8, 2025

Choose a reason for hiding this comment

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

The test references lastUpdateTime2DaysAgo variable but this variable is not defined in the visible test context. This will cause a compilation error.

Copilot uses AI. Check for mistakes.

Copy link
Contributor

@estebanreyl estebanreyl left a comment

Choose a reason for hiding this comment

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

Overall LGTM, left some minor comments.

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.

3 participants