FAQ | This is a LIVE service | Changelog

Skip to content
Snippets Groups Projects

feat: add migration support for artifact registry

Merged Dr Rich Wareham requested to merge experiment-artifact-registry into master
3 unresolved threads

Add support for external docker registry URLs allowing one to specify the artifact registry URL and start migrating away from use of this module.

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
62 62 }
63
64 variable "external_repository_url" {
65 description = <<-EOI
66 Location of external repository to push image.
67
68 If you are using the artifact registry, this will be something like:
69 europe-west2-docker.pkg.dev/some-meta-project-id/default.
70
71 Required if use_external_repository is true.
72 EOI
73 default = null
74 type = string
75 }
76
77 variable "use_external_repository" {
  • Surely a non-null value in external_repository_url would be sufficient, rather than another var?

  • Ah, yes and no. The tl;dr is that terraform will moan trying to create a plan if external_repository_url depends on resources which have not yet been created.

    The longer story is below.

    There is a little subtlety in terraform which has bitten me before that the value of count and for_each must be known quite early on in the process. So, indeed, we don't need use_external_repository if external_repository_url is a static string but if it is the output of a not yet created resource then terraform will moan that it can't determine the value of the string before applying and so can't generate a plan.

    It hits me often enough that I now keep all conditions which end up turning into count = ... ? 1 : 0 as explicit booleans.

    You can see this pattern being used in other terraform modules where people have hard-won experience. An example which springs to mind is create_url_map from https://registry.terraform.io/modules/GoogleCloudPlatform/lb-http/google/latest/submodules/serverless_negs. Strictly speaking it is inferable from url_map. Indeed the docs say "[s]et to false if url_map variable is provided" implying that the variable is redundant but since url_map may depend on other resources, terraform may not always be able to determine its value at plan time.

    So, yes, if terraform worked perfectly we don't need this but just-often-enough it's the case that we need to keep the flag and value which that flag depends on separate.

    Edited by Dr Rich Wareham
  • Please register or sign in to reply
  • 60 60 default = {}
    61 61 type = map(string)
    62 62 }
    63
    64 variable "external_repository_url" {
    65 description = <<-EOI
    66 Location of external repository to push image.
    67
    68 If you are using the artifact registry, this will be something like:
    69 europe-west2-docker.pkg.dev/some-meta-project-id/default.
    • Comment on lines +68 to +69

      May be too much trouble... but would using a data.google_artifact_registry_repository save someone having to construct this URL to use the module?

    • Possibly. But then that would preclude the use of any other external repository. Or special cases where the external repository needs to be outside of europe-west2. (We've had examples where features launch only in europe-west1, for example.)

      I wanted to keep this change orthogonal to which external repository was being used since it's a generic escape hatch.

    • Please register or sign in to reply
  • mentioned in epic uis/devops#251

  • Ryan Kowalewski requested review from @rk725

    requested review from @rk725

  • Ryan Kowalewski approved this merge request

    approved this merge request

    • @rjw57, I had a quick look at this following the emails today and I'm happy with it as a stop-gap. I have approved but will leave it up to you to merge in case you want to wait for @rjg21 responses to the two open threads.

    • I'll merge now since, hopefully, usage of the escape hatch will be time limited so I'm not too worried if it's not 100% perfect.

    • Please register or sign in to reply
  • Dr Rich Wareham mentioned in commit c9afd7d4

    mentioned in commit c9afd7d4

  • Please register or sign in to reply
    Loading