FAQ | This is a LIVE service | Changelog

Skip to content
Snippets Groups Projects

GCP Cloud Run manager terraform module

Merged GCP Cloud Run manager terraform module
gcp-cloud-run-app into master
1 unresolved thread
Merged Dr Abraham Martin requested to merge gcp-cloud-run-app into master
1 unresolved thread

This module manages a Cloud Run-hosted container. It takes care of making sure the container is connected to a Cloud SQL instance and sets environment variables on the application.

Closes #1 (closed)

Edited by Dr Abraham Martin

Merge request reports

Pipeline #25619 passed

Stage: test

    Pipeline passed for 58f62e4d on gcp-cloud-run-app

    Merged by Dr Rich WarehamDr Rich Wareham 5 years ago (Mar 23, 2020 11:57am UTC)

    Loading

    Pipeline #25620 passed

    Stage: test

      Pipeline passed for 2ea2b9d7 on master

      Activity

      Filter activity
      • Approvals
      • Assignees & reviewers
      • Comments (from bots)
      • Comments (from users)
      • Commits & branches
      • Edits
      • Labels
      • Lock status
      • Mentions
      • Merge request status
      • Tracking
    • Dr Rich Wareham
    • Dr Abraham Martin added 1 commit

      added 1 commit

      Compare with previous version

    • Dr Abraham Martin added 1 commit

      added 1 commit

      • 27259b69 - GCP Cloud Run manager terraform module

      Compare with previous version

    • Dr Rich Wareham
      Dr Rich Wareham @rjw57 started a thread on an old version of the diff
      • variables.tf 0 → 100644
      37 description = "Maximum number of concurrent requests to an instance before it is auto-scaled. Defaults to 80 which is the maximum that Cloud Run allows"
      38 default = "80"
      39 }
      40
      41 variable "cpu_limit" {
      42 description = "CPU limit for the deployed container. Defaults to 1 CPU."
      43 default = "1000m"
      44 }
      45
      46 variable "memory_limit" {
      47 description = "Memory limit for the deployed container. Defaults to 512 MB."
      48 default = "512M"
      49 }
      50
      51 variable "webapp_open" {
      52 description = "By default Cloud Run does not allow non-authenticated traffic to the web app. This settings changes that default to be open to all traffic (value of 1). Change it to 0 if you don't want to change default behaviour."
      • Dr Rich Wareham
        Dr Rich Wareham @rjw57 ·
        Owner

        Is there a pressing reason not to have this be something like:

        variable "webapp_open" {
  description = <<EOI
If true, the webapp will allow unauthenticated invocations. If false, the webapp requires authentication
as a Google user with the Cloud Run invoker permission on the deployment.
EOI
  type    = bool
  default = true
}

        After all, we have booleans. Let's use them.

        Edited by Dr Rich Wareham
      • Dr Abraham Martin
        Dr Abraham Martin @amc203 ·
        Author Owner

        the 1 is to be used with count

      • Dr Abraham Martin changed this line in version 4 of the diff

        changed this line in version 4 of the diff

      • Dr Rich Wareham
        Dr Rich Wareham @rjw57 ·
        Owner

        Yes, but that doesn't mean we need to have an unfriendly interface. You could turn the boolean into a count and hide the implementation detail from the caller:

        # Allow unauthenticated invocations for the webapp.
resource "google_cloud_run_service_iam_member" "webapp_all_users_invoker" {
  count    = var.webapp_open ? 1 : 0
  location = google_cloud_run_service.webapp.location
  project  = google_cloud_run_service.webapp.project
  service  = google_cloud_run_service.webapp.name
  role     = "roles/run.invoker"
  member   = "allUsers"
}

        Aside from not being a friendly interface, having it be numeric guarantees someone will set it to "2" by mistake.

      • Dr Rich Wareham
        Dr Rich Wareham @rjw57 ·
        Owner

        Also, I'd change the name of the variable to allow_unauthenticated_invocations which is a bit more explicit about what it does.

      • Dr Abraham Martin
        Dr Abraham Martin @amc203 ·
        Author Owner

        fixed

      • Dr Rich Wareham
        Dr Rich Wareham @rjw57 ·
        Owner

        fixed

        I still think the variable should be re-named and be a boolean. It's a weird interface to have have boolean value passed as a number.

      • Dr Abraham Martin
        Dr Abraham Martin @amc203 ·
        Author Owner

        renamed

      • Please register or sign in to reply
    • Dr Rich Wareham
    • Dr Rich Wareham
    • Dr Abraham Martin added 1 commit

      added 1 commit

      Compare with previous version

    • Dr Abraham Martin changed the description

      changed the description

    • Dr Abraham Martin added 1 commit

      added 1 commit

      Compare with previous version

    • Dr Abraham Martin added 1 commit

      added 1 commit

      Compare with previous version

    • Dr Abraham Martin added 1 commit

      added 1 commit

      Compare with previous version

    • Dr Rich Wareham approved this merge request

      approved this merge request

    • Dr Rich Wareham enabled an automatic merge when the pipeline for 58f62e4d succeeds

      enabled an automatic merge when the pipeline for 58f62e4d succeeds

    • Dr Rich Wareham mentioned in commit 2ea2b9d7

      mentioned in commit 2ea2b9d7

    • Dr Rich Wareham merged

      merged

      • Please register or sign in to reply