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.

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

Refs platform/#3202 #13

Merged
merged 27 commits into from
Oct 16, 2024
Merged

Refs platform/#3202 #13

merged 27 commits into from
Oct 16, 2024

Conversation

Stevesibilia
Copy link
Contributor

@Stevesibilia Stevesibilia commented Oct 15, 2024

PR Type

Enhancement


Description

  • Implemented support for GCP Secret Manager to handle passwords for remote repositories
  • Added new username_password_credentials_password_secret_name field in the remote_repository_config_docker object
  • Updated google_artifact_registry_repository resource to use Secret Manager for password retrieval
  • Introduced validation to ensure remote repository configuration is provided when using REMOTE_REPOSITORY mode
  • Added local variable remote_repositories to manage remote repository configurations
  • Updated CHANGELOG to reflect the breaking change and new version 0.7.0

Changes walkthrough 📝

Relevant files
Enhancement
main.tf
Implement remote repository secret management                       

main.tf

  • Added remote_repositories local variable to handle remote repository
    configurations
  • Introduced google_secret_manager_secret_version data source for remote
    repository secrets
  • Updated google_artifact_registry_repository resource to use secret
    manager for password
  • +22/-2   
    variables.tf
    Update variables and add validation for remote repositories

    variables.tf

  • Added username_password_credentials_password_secret_name to
    remote_repository_config_docker object
  • Introduced validation for remote repository configuration
  • +6/-0     
    Documentation
    CHANGELOG.md
    Update CHANGELOG for version 0.7.0                                             

    CHANGELOG.md

  • Added entry for version 0.7.0
  • Documented breaking change for remote repository password management
  • +6/-0     

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

    @sparkfabrik-ai-bot
    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Potential Error
    The for_each condition in the google_secret_manager_secret_version data source might cause issues if username_password_credentials_password_secret_name is provided but username_password_credentials_username is empty.

    Unused Variable
    The username_password_credentials_password_secret_version variable is still defined in variables.tf but not used in the main configuration.

    Incomplete Validation
    The validation for remote repository configuration doesn't check if both username and password secret name are provided when mode is REMOTE_REPOSITORY.

    @sparkfabrik-ai-bot
    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Replace lookup function with coalesce for improved readability

    Consider using coalesce() function instead of lookup() with default values for
    better readability and consistency.

    main.tf [39-41]

    -username_password_credentials_username                = lookup(repository.remote_repository_config_docker, "username_password_credentials_username", "")
    -username_password_credentials_password_secret_name    = lookup(repository.remote_repository_config_docker, "username_password_credentials_password_secret_name", "")
    -username_password_credentials_password_secret_version = lookup(repository.remote_repository_config_docker, "username_password_credentials_password_secret_version", "latest")
    +username_password_credentials_username                = coalesce(repository.remote_repository_config_docker.username_password_credentials_username, "")
    +username_password_credentials_password_secret_name    = coalesce(repository.remote_repository_config_docker.username_password_credentials_password_secret_name, "")
    +username_password_credentials_password_secret_version = coalesce(repository.remote_repository_config_docker.username_password_credentials_password_secret_version, "latest")
     
    Suggestion importance[1-10]: 6

    Why: The suggestion improves code readability by using coalesce(), which is more idiomatic in Terraform. However, it's a minor improvement and doesn't address any critical issues.

    6
    Maintainability
    Improve readability of complex condition by formatting it across multiple lines

    Consider using a more descriptive name for the alltrue condition to improve code
    readability.

    main.tf [50]

    -if alltrue([value.username_password_credentials_username != "", value.username_password_credentials_password_secret_name != ""])
    +if alltrue([
    +  value.username_password_credentials_username != "",
    +  value.username_password_credentials_password_secret_name != ""
    +])
     
    Suggestion importance[1-10]: 5

    Why: The suggestion enhances code readability by breaking a long condition into multiple lines. While helpful, it's a minor formatting change that doesn't significantly impact functionality.

    5
    Improve variable naming in complex condition for better code clarity

    Consider using a more descriptive variable name for cp in the flatten function to
    improve code readability.

    variables.tf [72]

    -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])
    +condition     = alltrue([for policy in flatten([for repo in var.repositories : [for cleanup_policy in repo.cleanup_policies : cleanup_policy]]) : policy.most_recent_versions == {} || policy.most_recent_versions.keep_count == null || policy.most_recent_versions.keep_count >= 0])
     
    Suggestion importance[1-10]: 4

    Why: The suggestion proposes renaming a variable for better clarity. While it slightly improves code readability, the impact is minimal and doesn't address any functional issues.

    4

    @Stevesibilia Stevesibilia merged commit 2f0d900 into main Oct 16, 2024
    1 check passed
    @Stevesibilia Stevesibilia deleted the refs-platform/#3202 branch October 16, 2024 08:24
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant