platform/#3101: add cleanup policies management

[Monska85]

PR Type

Enhancement

---

Description

• Implemented cleanup policies management for Artifact Registry repositories
• Added dynamic blocks in main.tf to handle cleanup_policies, including nested blocks for condition and most_recent_versions
• Extended the repositories variable in variables.tf to include cleanup_policies configuration
• Added validation rules for cleanup policy action, tag state, and keep count
• Updated CHANGELOG.md and README.md to reflect the new cleanup policies feature
• Removed docker_immutable_tags from the optional parameters list in variables.tf

---

Changes walkthrough 📝

	Relevant files
Enhancement	main.tf Implement cleanup policies for Artifact Registry repositories main.tf Added dynamic block for cleanup_policies in google_artifact_registry_repository resource Implemented nested dynamic blocks for condition and most_recent_versions within cleanup_policies Added logic to handle optional cleanup policy configurations +28/-0    variables.tf Add cleanup policies configuration and validation                variables.tf Extended repositories variable to include cleanup_policies configuration Added validation rules for cleanup policy action, tag state, and keep count Removed docker_immutable_tags from the optional parameters list +31/-1
Documentation	CHANGELOG.md Update CHANGELOG with cleanup policies feature                      CHANGELOG.md Added entry for new feature: cleanup policies management for repositories +4/-0      README.md Update README with new cleanup policies input                        README.md Updated input variables table to include cleanup_policies configuration +1/-1

---

💡 PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

[sparkfabrik-ai-bot]

PR Reviewer Guide 🔍

(Review updated until commit 573d545)

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Key issues to review Validation Complexity: The validation rules in variables.tf might not cover all possible edge cases for cleanup policies. Consider adding more comprehensive validation. Error Handling: There's no explicit error handling for potential issues that may arise during the creation of cleanup policies. Consider adding appropriate error handling mechanisms. Documentation: While README.md is updated, it might benefit from more detailed explanations of the new cleanup policies feature and its configuration options.

[sparkfabrik-ai-bot]

PR Code Suggestions ✨

Latest suggestions up to c3490fc

Category	Suggestion	Score
Possible issue	Ensure 'keep_count' is positive when 'most_recent_versions' is specified Add a validation rule to ensure that when 'most_recent_versions' is specified, 'keep_count' is greater than zero to avoid potential issues with keeping zero versions. variables.tf [70-73] validation { - condition = alltrue([for policy in flatten([for repo in var.repositories : [for cp in repo.cleanup_policies : cp]]) : policy.most_recent_versions == {} || policy.most_recent_versions.keep_count == null || policy.most_recent_versions.keep_count >= 0]) - error_message = "Keep count must be a non-negative number." + condition = alltrue([for policy in flatten([for repo in var.repositories : [for cp in repo.cleanup_policies : cp]]) : policy.most_recent_versions == {} || policy.most_recent_versions.keep_count == null || policy.most_recent_versions.keep_count > 0]) + error_message = "Keep count must be a positive number when most_recent_versions is specified." } Suggestion importance[1-10]: 9 Why: This suggestion addresses a potential logical error in the cleanup policy configuration, ensuring that at least one version is kept when specified.	9
	Validate that at least one field is set in the condition object when specified Add a validation rule to ensure that when 'condition' is specified, at least one of its fields is set to avoid creating an empty condition. variables.tf [28-35] condition = optional(object({ tag_state = optional(string), tag_prefixes = optional(list(string), []), version_name_prefixes = optional(list(string), []), package_name_prefixes = optional(list(string), []), older_than = optional(string), newer_than = optional(string), }), {}) +validation { + condition = alltrue([for policy in flatten([for repo in var.repositories : [for cp in repo.cleanup_policies : cp]]) : policy.condition == {} || length(compact([policy.condition.tag_state, length(policy.condition.tag_prefixes), length(policy.condition.version_name_prefixes), length(policy.condition.package_name_prefixes), policy.condition.older_than, policy.condition.newer_than])) > 0]) + error_message = "At least one field in the condition object must be set when condition is specified." +} + Suggestion importance[1-10]: 8 Why: This validation prevents the creation of empty conditions, which could lead to unexpected behavior in the cleanup policies.	8
Maintainability	Use a separate resource for cleanup policies instead of a dynamic block Consider using a separate resource block for cleanup policies instead of a dynamic block. This can improve readability and make it easier to manage individual policies. main.tf [47-73] -dynamic "cleanup_policies" { +resource "google_artifact_registry_repository_cleanup_policy" "policy" { for_each = each.value.cleanup_policies - content { - id = cleanup_policies.key - action = cleanup_policies.value.action - ... + repository = google_artifact_registry_repository.repositories[each.key].name + location = google_artifact_registry_repository.repositories[each.key].location + project = var.project_id + + policy_id = each.key + action = each.value.action + + condition { + tag_state = each.value.condition.tag_state + tag_prefixes = each.value.condition.tag_prefixes + version_name_prefixes = each.value.condition.version_name_prefixes + package_name_prefixes = each.value.condition.package_name_prefixes + older_than = each.value.condition.older_than + newer_than = each.value.condition.newer_than + } + + most_recent_versions { + package_name_prefixes = each.value.most_recent_versions.package_name_prefixes + keep_count = each.value.most_recent_versions.keep_count } } Suggestion importance[1-10]: 7 Why: Using separate resources can improve readability and manageability, but the current dynamic block approach is also valid and more concise.	7
Error handling	Add error handling for invalid cleanup policy configurations Consider adding error handling or logging for cases where the cleanup policy configuration might be invalid or incomplete. main.tf [47-73] dynamic "cleanup_policies" { for_each = each.value.cleanup_policies content { id = cleanup_policies.key action = cleanup_policies.value.action dynamic "condition" { for_each = cleanup_policies.value.condition != {} ? [cleanup_policies.value.condition] : [] content { tag_state = condition.value.tag_state tag_prefixes = condition.value.tag_prefixes version_name_prefixes = condition.value.version_name_prefixes package_name_prefixes = condition.value.package_name_prefixes older_than = condition.value.older_than newer_than = condition.value.newer_than } } dynamic "most_recent_versions" { for_each = cleanup_policies.value.most_recent_versions != {} && cleanup_policies.value.most_recent_versions.keep_count != 0 ? [cleanup_policies.value.most_recent_versions] : [] content { package_name_prefixes = most_recent_versions.value.package_name_prefixes keep_count = most_recent_versions.value.keep_count } } } + + lifecycle { + precondition { + condition = cleanup_policies.value.condition != {} || cleanup_policies.value.most_recent_versions != {} + error_message = "Either 'condition' or 'most_recent_versions' must be specified for cleanup policy '${cleanup_policies.key}'." + } + } } Suggestion importance[1-10]: 6 Why: While adding error handling is beneficial, the existing validation rules in variables.tf already cover most cases. This suggestion provides an additional layer of safety.	6

---

Previous suggestions

Suggestions up to commit 573d545

Category	Suggestion	Score
Possible bug	Correct the attribute name in the validation block for consistency with the variable definition The validation block for the most_recent_version.keep_count is using an incorrect attribute name. It should be most_recent_versions (plural) instead of most_recent_version (singular) to match the variable definition. variables.tf [69-72] validation { - condition = alltrue([for policy in [for repo in var.repositories : repo.cleanup_policies] : policy.most_recent_version.keep_count == null || policy.most_recent_version.keep_count >= 0]) + condition = alltrue([for policy in [for repo in var.repositories : repo.cleanup_policies] : policy.most_recent_versions.keep_count == null || policy.most_recent_versions.keep_count >= 0]) error_message = "Keep count must be a non-negative number." } Suggestion importance[1-10]: 9 Why: This suggestion fixes a potential bug by correcting the attribute name from 'most_recent_version' to 'most_recent_versions', which is crucial for proper validation.	9
Possible issue	Correct the validation logic to properly check all cleanup policies across all repositories The validation for cleanup policy actions is not correctly implemented. It should iterate over all policies for all repositories, not just the first policy of each repository. variables.tf [59-62] validation { - condition = alltrue([for policy in [for repo in var.repositories : repo.cleanup_policies] : contains(["DELETE", "KEEP"], policy.action)]) + condition = alltrue(flatten([for repo in var.repositories : [for policy in repo.cleanup_policies : contains(["DELETE", "KEEP"], policy.action)]])) error_message = "Cleanup policy action must be either DELETE or KEEP." } Suggestion importance[1-10]: 9 Why: This suggestion fixes a significant issue in the validation logic, ensuring all cleanup policies are properly checked, which is critical for correct functionality.	9
	Improve the validation logic for tag state to handle all cases and check all policies The validation for tag state is not correctly implemented. It should iterate over all policies for all repositories, not just the first policy of each repository. Also, it should handle the case where the condition might not be defined. variables.tf [64-67] validation { - condition = alltrue([for policy in [for repo in var.repositories : repo.cleanup_policies] : policy.condition.tag_state == null || contains(["ANY", "TAGGED", "UNTAGGED"], policy.condition.tag_state)]) + condition = alltrue(flatten([for repo in var.repositories : [for policy in repo.cleanup_policies : policy.condition == null || policy.condition.tag_state == null || contains(["ANY", "TAGGED", "UNTAGGED"], policy.condition.tag_state)]])) error_message = "Tag state must be ANY, TAGGED, or UNTAGGED." } Suggestion importance[1-10]: 9 Why: This suggestion addresses an important issue in the validation logic, ensuring all policies are checked and handling cases where the condition might not be defined, which is crucial for robust validation.	9
Maintainability	Refactor cleanup policies structure to improve code organization and reusability Consider using a separate function or local variable to define the cleanup policies structure. This will improve readability and maintainability of the code, especially if the structure is used in multiple places. main.tf [47-73] dynamic "cleanup_policies" { for_each = each.value.cleanup_policies content { id = cleanup_policies.key action = cleanup_policies.value.action dynamic "condition" { for_each = cleanup_policies.value.condition != {} ? [cleanup_policies.value.condition] : [] - content { - tag_state = condition.value.tag_state - tag_prefixes = condition.value.tag_prefixes - version_name_prefixes = condition.value.version_name_prefixes - package_name_prefixes = condition.value.package_name_prefixes - older_than = condition.value.older_than - newer_than = condition.value.newer_than - } + content = local.cleanup_policy_condition_content } dynamic "most_recent_versions" { for_each = cleanup_policies.value.most_recent_versions != {} && cleanup_policies.value.most_recent_versions.keep_count != 0 ? [cleanup_policies.value.most_recent_versions] : [] - content { - package_name_prefixes = most_recent_versions.value.package_name_prefixes - keep_count = most_recent_versions.value.keep_count - } + content = local.cleanup_policy_most_recent_versions_content } } } +# Define locals +locals { + cleanup_policy_condition_content = { + tag_state = condition.value.tag_state + tag_prefixes = condition.value.tag_prefixes + version_name_prefixes = condition.value.version_name_prefixes + package_name_prefixes = condition.value.package_name_prefixes + older_than = condition.value.older_than + newer_than = condition.value.newer_than + } + + cleanup_policy_most_recent_versions_content = { + package_name_prefixes = most_recent_versions.value.package_name_prefixes + keep_count = most_recent_versions.value.keep_count + } +} + Suggestion importance[1-10]: 6 Why: The suggestion improves code maintainability by extracting repeated structures into local variables, but it's not addressing a critical issue or bug.	6

Suggestions up to commit 1777dbd

Category	Suggestion	Score
Breaking change	Document the relocation of the docker_immutable_tags variable within the repositories map The docker_immutable_tags variable has been moved within the repositories map. Ensure this change is documented and that any existing configurations using this variable are updated accordingly. variables.tf [26-42] +cleanup_policies = optional(map(object({ + ... +})), {}) +docker_immutable_tags = optional(bool, true) +virtual_repository_config = optional(map(object({ - Suggestion importance[1-10]: 8 Why: This suggestion highlights an important change that could affect existing configurations. Proper documentation of such changes is crucial for maintaining backwards compatibility and user awareness.	8
Readability	Simplify and clarify the condition for the most_recent_versions dynamic block The condition for most_recent_versions dynamic block is complex and may be difficult to understand. Consider simplifying it or adding a comment to explain the logic. main.tf [65-71] +# Only create most_recent_versions block if it's not empty and keep_count is non-zero dynamic "most_recent_versions" { - for_each = cleanup_policies.value.most_recent_versions != {} && cleanup_policies.value.most_recent_versions.keep_count != 0 ? [cleanup_policies.value.most_recent_versions] : [] + for_each = ( + cleanup_policies.value.most_recent_versions != {} && + try(cleanup_policies.value.most_recent_versions.keep_count, 0) > 0 + ) ? [cleanup_policies.value.most_recent_versions] : [] content { package_name_prefixes = most_recent_versions.value.package_name_prefixes keep_count = most_recent_versions.value.keep_count } } Suggestion importance[1-10]: 7 Why: The suggestion improves code readability by simplifying the condition and adding a clarifying comment, which is beneficial for maintainability.	7
Maintainability	Use a separate resource for cleanup policies instead of a dynamic block Consider using a separate resource block for cleanup policies instead of a dynamic block. This can improve readability and make it easier to manage individual cleanup policies. main.tf [47-73] -dynamic "cleanup_policies" { +resource "google_artifact_registry_repository_cleanup_policy" "policy" { for_each = each.value.cleanup_policies - content { - id = cleanup_policies.key - action = cleanup_policies.value.action - ... + repository = google_artifact_registry_repository.repositories[each.key].name + location = google_artifact_registry_repository.repositories[each.key].location + project = var.project_id + + policy_id = each.key + action = each.value.action + + condition { + tag_state = each.value.condition.tag_state + tag_prefixes = each.value.condition.tag_prefixes + version_name_prefixes = each.value.condition.version_name_prefixes + package_name_prefixes = each.value.condition.package_name_prefixes + older_than = each.value.condition.older_than + newer_than = each.value.condition.newer_than + } + + most_recent_versions { + package_name_prefixes = each.value.most_recent_versions.package_name_prefixes + keep_count = each.value.most_recent_versions.keep_count } } Suggestion importance[1-10]: 6 Why: While using a separate resource can improve readability, the current dynamic block approach is valid and may be more concise for simple configurations. The suggestion is good but not crucial.	6
Usability	Flatten the cleanup_policies variable structure for easier use The cleanup_policies variable is deeply nested and might be difficult to use. Consider flattening the structure or providing helper variables for common configurations. variables.tf [26-40] cleanup_policies = optional(map(object({ - action = optional(string, ""), - condition = optional(object({ - tag_state = optional(string), - tag_prefixes = optional(list(string)), - version_name_prefixes = optional(list(string)), - package_name_prefixes = optional(list(string)), - older_than = optional(string), - newer_than = optional(string), - }), {}), - most_recent_versions = optional(object({ - package_name_prefixes = optional(list(string)), - keep_count = optional(number, 0) - }), {}) + action = optional(string, "") + condition_tag_state = optional(string) + condition_tag_prefixes = optional(list(string)) + condition_version_prefixes = optional(list(string)) + condition_package_prefixes = optional(list(string)) + condition_older_than = optional(string) + condition_newer_than = optional(string) + keep_recent_count = optional(number, 0) + keep_recent_packages = optional(list(string)) })), {}) Suggestion importance[1-10]: 5 Why: While flattening the structure could make it easier to use, the current nested structure is valid and may be preferred for organizing complex configurations. The suggestion is a matter of preference.	5

Suggestions up to commit 95f37be

Category	Suggestion	Score
Enhancement	Add validation blocks for cleanup policy variables Add validation blocks for the cleanup_policies variable to ensure that the provided values are within acceptable ranges or formats. variables.tf [26-40] cleanup_policies = optional(map(object({ - action = optional(string, ""), + action = optional(string, "DELETE"), condition = optional(object({ tag_state = optional(string, "ANY"), - tag_prefixes = optional(string, ""), - version_name_prefixes = optional(string, ""), - package_name_prefixes = optional(string, ""), + tag_prefixes = optional(list(string), []), + version_name_prefixes = optional(list(string), []), + package_name_prefixes = optional(list(string), []), older_than = optional(string, ""), newer_than = optional(string, ""), }), {}), most_recent_version = optional(object({ - package_name_prefixes = optional(string, "") + package_name_prefixes = optional(list(string), []) keep_count = optional(number, 0) }), {}) })), {}) +validation { + condition = alltrue([for policy in var.cleanup_policies : contains(["DELETE", "KEEP"], policy.action)]) + error_message = "Cleanup policy action must be either DELETE or KEEP." +} + +validation { + condition = alltrue([for policy in var.cleanup_policies : policy.condition.tag_state == null || contains(["ANY", "TAGGED", "UNTAGGED"], policy.condition.tag_state)]) + error_message = "Tag state must be ANY, TAGGED, or UNTAGGED." +} + +validation { + condition = alltrue([for policy in var.cleanup_policies : policy.most_recent_version.keep_count == null || policy.most_recent_version.keep_count >= 0]) + error_message = "Keep count must be a non-negative number." +} + Suggestion importance[1-10]: 9 Why: Adding validation blocks is crucial for ensuring data integrity and preventing runtime errors. This suggestion significantly enhances the robustness of the code.	9
Best practice	Use more specific types for cleanup policy variables Consider using more specific types for the cleanup_policies map. Instead of using optional(string, "") for action, use a list of allowed values to prevent potential errors. variables.tf [26-40] cleanup_policies = optional(map(object({ - action = optional(string, ""), + action = optional(string, "DELETE"), condition = optional(object({ tag_state = optional(string, "ANY"), - tag_prefixes = optional(string, ""), - version_name_prefixes = optional(string, ""), - package_name_prefixes = optional(string, ""), + tag_prefixes = optional(list(string), []), + version_name_prefixes = optional(list(string), []), + package_name_prefixes = optional(list(string), []), older_than = optional(string, ""), newer_than = optional(string, ""), }), {}), most_recent_version = optional(object({ - package_name_prefixes = optional(string, "") + package_name_prefixes = optional(list(string), []) keep_count = optional(number, 0) }), {}) })), {}) Suggestion importance[1-10]: 8 Why: Using more specific types, especially for lists and enums, can prevent errors and improve code quality. This suggestion offers valuable improvements to the variable definitions.	8
Maintainability	Use a separate resource for cleanup policies instead of a dynamic block Consider using a separate resource block for cleanup policies instead of a dynamic block. This can improve readability and make it easier to manage individual cleanup policies. main.tf [47-73] -dynamic "cleanup_policies" { +resource "google_artifact_registry_repository_cleanup_policy" "policy" { for_each = each.value.cleanup_policies - content { - id = cleanup_policies.key - action = cleanup_policies.value.action - ... + repository = google_artifact_registry_repository.repositories[each.key].name + location = google_artifact_registry_repository.repositories[each.key].location + project = var.project_id + + policy_id = each.key + action = each.value.action + + condition { + tag_state = each.value.condition.tag_state + tag_prefixes = each.value.condition.tag_prefixes + version_name_prefixes = each.value.condition.version_name_prefixes + package_name_prefixes = each.value.condition.package_name_prefixes + older_than = each.value.condition.older_than + newer_than = each.value.condition.newer_than + } + + most_recent_versions { + package_name_prefixes = each.value.most_recent_version.package_name_prefixes + keep_count = each.value.most_recent_version.keep_count } } Suggestion importance[1-10]: 7 Why: Using separate resources for cleanup policies can improve readability and manageability, but the current dynamic block approach is also valid and more concise.	7

[sparkfabrik-ai-bot]

Persistent review updated to latest commit 1777dbd

[paolomainardi]

LGTM

[sparkfabrik-ai-bot]

Persistent review updated to latest commit 573d545

[Monska85]

/improve

[paolomainardi]

LGTM