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

Platform#3202 #12

Merged
merged 3 commits into from
Oct 9, 2024
Merged

Platform#3202 #12

merged 3 commits into from
Oct 9, 2024

Conversation

Stevesibilia
Copy link
Contributor

@Stevesibilia Stevesibilia commented Oct 9, 2024

User description

Terraform will perform the following actions:

  # module.artifact_repositories.google_artifact_registry_repository.repositories["docker-hub-mirror"] will be created
  + resource "google_artifact_registry_repository" "repositories" {
      + cleanup_policy_dry_run = false
      + create_time            = (known after apply)
      + description            = "Artifact registry repository in remote mode to proxy Docker Hub registry"
      + effective_labels       = (known after apply)
      + format                 = "DOCKER"
      + id                     = (known after apply)
      + location               = "europe-west1"
      + mode                   = "REMOTE_REPOSITORY"
      + name                   = (known after apply)
      + project                = "spark-int-cloud-services"
      + repository_id          = "docker-hub-mirror"
      + terraform_labels       = (known after apply)
      + update_time            = (known after apply)

      + cleanup_policies {
          + action = "DELETE"
          + id     = "remove-older-90d"

          + condition {
              + older_than            = "7776000s"
              + package_name_prefixes = []
              + tag_prefixes          = []
              + tag_state             = "ANY"
              + version_name_prefixes = []
            }
        }

      + remote_repository_config {
          + description                 = "docker hub with custom credentials"
          + disable_upstream_validation = true

          + docker_repository {
              + public_repository = "DOCKER_HUB"
            }

          + upstream_credentials {
              + username_password_credentials {
                  + password_secret_version = "1"
                  + username                = "sparkfabrik"
                }
            }
        }
    }

  # module.artifact_repositories.google_artifact_registry_repository_iam_member.member["docker-hub-mirror--roles/artifactregistry.writer--serviceAccount:[email protected]"] will be created
  + resource "google_artifact_registry_repository_iam_member" "member" {
      + etag       = (known after apply)
      + id         = (known after apply)
      + location   = "europe-west1"
      + member     = "serviceAccount:[email protected]"
      + project    = "spark-int-cloud-services"
      + repository = (known after apply)
      + role       = "roles/artifactregistry.writer"
    }


PR Type

Enhancement


Description

  • Added support for mirroring Docker Hub as a public registry in the Artifact Registry
  • Implemented dynamic blocks in Terraform configuration to handle both custom and public Docker repositories
  • Updated main.tf to use conditional logic based on the custom_repository_uri value
  • Added version 0.6.0 entry in CHANGELOG.md to document the new feature
  • Improved flexibility of the module to support different repository configurations

Changes walkthrough 📝

Relevant files
Enhancement
main.tf
Add Docker Hub mirroring support                                                 

main.tf

  • Added support for mirroring Docker Hub as a public registry
  • Implemented dynamic blocks for custom and public Docker repositories
  • Conditional logic based on custom_repository_uri value
  • +13/-3   
    Documentation
    CHANGELOG.md
    Update CHANGELOG for version 0.6.0                                             

    CHANGELOG.md

  • Added entry for version 0.6.0
  • Documented new feature for mirroring Docker Hub
  • +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: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Code Duplication
    The docker_repository block is duplicated with slight variations. Consider refactoring to reduce duplication and improve maintainability.

    Potential Logic Error
    The condition for the second docker_repository block might be redundant. If the first block doesn't match, the second will always match. Consider simplifying the logic.

    @sparkfabrik-ai-bot
    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Documentation
    Expand the changelog entry with more detailed information about the new feature and its implications

    Consider adding more details about the new feature in the changelog, such as any
    configuration changes required or potential impacts on existing setups.

    CHANGELOG.md [23-25]

     ## [0.6.0] - 2024-10-09
     
    -- FEATURE: add support for mirror public registry (Docker Hub) if `custom_repository_uri` is `DOCKER_HUB`
    +### Added
    +- Support for mirroring public Docker Hub registry
    +  - Set `custom_repository_uri` to `DOCKER_HUB` to enable this feature
    +  - This allows users to easily mirror Docker Hub images in their Artifact Registry
     
    +### Changed
    +- Modified `docker_repository` configuration to support both custom and public repositories
    +
    +### Notes
    +- Existing configurations using custom repositories will continue to work as before
    +- Users can now choose between custom repositories and Docker Hub mirroring
    +
    Suggestion importance[1-10]: 8

    Why: The suggestion significantly improves the documentation by providing more comprehensive and structured information about the new feature, which is crucial for users and maintainers.

    8
    Maintainability
    Introduce a local variable for a condition used multiple times to improve code clarity

    The condition remote_repository_config.value.custom_repository_uri != "DOCKER_HUB"
    is used twice with opposite logic. Consider defining a local variable for this
    condition to improve readability and reduce the chance of errors.

    main.tf [94-103]

    -for_each = remote_repository_config.value.custom_repository_uri != "DOCKER_HUB" ? [remote_repository_config.value] : []
    +locals {
    +  is_docker_hub = remote_repository_config.value.custom_repository_uri == "DOCKER_HUB"
    +}
     ...
    -for_each = remote_repository_config.value.custom_repository_uri == "DOCKER_HUB" ? [remote_repository_config.value] : []
    +for_each = local.is_docker_hub ? [] : [remote_repository_config.value]
    +...
    +for_each = local.is_docker_hub ? [remote_repository_config.value] : []
     
    Suggestion importance[1-10]: 7

    Why: This suggestion improves code readability and maintainability by reducing repetition of a complex condition, which is a good practice in Terraform configurations.

    7
    Enhancement
    Combine two dynamic blocks into a single dynamic block with conditional content

    Consider using a single dynamic block with a conditional inside instead of two
    separate dynamic blocks for docker_repository. This can make the code more concise
    and easier to maintain.

    main.tf [93-107]

     dynamic "docker_repository" {
    -  for_each = remote_repository_config.value.custom_repository_uri != "DOCKER_HUB" ? [remote_repository_config.value] : []
    +  for_each = [remote_repository_config.value]
       content {
    -    custom_repository {
    -      uri = remote_repository_config.value.custom_repository_uri
    +    dynamic "custom_repository" {
    +      for_each = content.value.custom_repository_uri != "DOCKER_HUB" ? [content.value] : []
    +      content {
    +        uri = custom_repository.value.custom_repository_uri
    +      }
    +    }
    +    dynamic "public_repository" {
    +      for_each = content.value.custom_repository_uri == "DOCKER_HUB" ? ["DOCKER_HUB"] : []
    +      content {
    +        public_repository = public_repository.value
    +      }
         }
       }
     }
     
    -dynamic "docker_repository" {
    -  for_each = remote_repository_config.value.custom_repository_uri == "DOCKER_HUB" ? [remote_repository_config.value] : []
    -  content {
    -    public_repository = "DOCKER_HUB"
    -  }
    -}
    -
    Suggestion importance[1-10]: 6

    Why: The suggestion offers a valid optimization to reduce code duplication, but the current structure is already clear and the improvement is not crucial for functionality.

    6

    CHANGELOG.md Outdated
    Comment on lines 25 to 27
    - FEATURE: add support for mirror public registry (Docker Hub) if `custom_repository_uri` is `DOCKER_HUB`

    [Compare with previous version](https://github.com/sparkfabrik/terraform-google-gcp-artifact-registry/compare/0.5.0...0.6.0)
    Copy link
    Contributor

    @Monska85 Monska85 Oct 9, 2024

    Choose a reason for hiding this comment

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

    Suggested change
    - FEATURE: add support for mirror public registry (Docker Hub) if `custom_repository_uri` is `DOCKER_HUB`
    [Compare with previous version](https://github.com/sparkfabrik/terraform-google-gcp-artifact-registry/compare/0.5.0...0.6.0)
    [Compare with previous version](https://github.com/sparkfabrik/terraform-google-gcp-artifact-registry/compare/0.5.0...0.6.0)
    ### Added
    - FEATURE: add support for mirror public registry (Docker Hub) if `custom_repository_uri` is `DOCKER_HUB`

    Keep the compare link at the beginning of the release section.

    @Stevesibilia Stevesibilia merged commit bb1ad0f into main Oct 9, 2024
    1 check passed
    @Stevesibilia Stevesibilia deleted the platform#3202 branch October 9, 2024 14:16
    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.

    2 participants