Cloud functions #899
Cloud functions #899
Conversation
Create is limited to gs:// source now.
|
I have fixed missing vendor package for cloudfunctions. |
There was a problem hiding this comment.
Thanks for the PR @sarunask! There's a lot of code here, so apologies in advance that there'll probably be a lot of back and forth before this gets in. Here's the first batch of comments that I have. I haven't gotten around to looking at everything yet, but I think this is a really good starting point.
| "description": { | ||
| Type: schema.TypeString, | ||
| Optional: true, | ||
| Elem: &schema.Schema{Type: schema.TypeString}, |
There was a problem hiding this comment.
This line isn't necessary, you don't need an Elem for strings (this applies for the rest of the fields as well)
| Type: schema.TypeMap, | ||
| Optional: true, | ||
| Elem: &schema.Schema{Type: schema.TypeString}, | ||
| Set: schema.HashString, |
There was a problem hiding this comment.
No need for Set on a TypeMap
|
|
||
| "storage_bucket": { | ||
| Type: schema.TypeString, | ||
| Required: true, |
There was a problem hiding this comment.
We typically sort our fields in the schema in the order Required -> Optional -> Computed- it makes it a little easier to navigate.
| return err | ||
| } | ||
|
|
||
| if FUNCTION_ALLOWED_REGION[region] != true { |
There was a problem hiding this comment.
Instead of this block, you can add a ValidateFunc into the schema for region. That'll catch these sorts of errors on plan-time instead of on apply-time. There's one that you can call called stringInSlice or something like that, look around for examples
|
|
||
| if v, ok := d.GetOk("memory"); ok { | ||
| memory := v.(int) | ||
| if FUNCTION_ALLOWED_MEMORY[memory] != true { |
There was a problem hiding this comment.
same thing here with a ValidateFunc
| func resourceCloudFunctionsCreate(d *schema.ResourceData, meta interface{}) error { | ||
| config := meta.(*Config) | ||
|
|
||
| service := config.clientCloudFunctions |
There was a problem hiding this comment.
this is fine, but since you're only using the service variable once you may as well inline it into the request below (that'll match the rest of the resources in this repo as well)
| return err | ||
| } | ||
|
|
||
| name := d.Get("name").(string) |
There was a problem hiding this comment.
If you use d.Id() here instead of d.Get("name").(string), then you can use an ImportStatePassthrough importer instead of a custom one
| if err != nil { | ||
| return fmt.Errorf("Error while updating cloudfunction configuration: %s", err) | ||
| } | ||
| for i := range partialArr { |
There was a problem hiding this comment.
This would only be necessary if you were sending multiple requests. Since you're only sending one, no need for SetPartial
| } | ||
| } | ||
|
|
||
| func dataSourceGoogleCloudFunctionsFunctionRead(d *schema.ResourceData, meta interface{}) error { |
There was a problem hiding this comment.
Is this significantly different from the resource read function? If not, you should just call that one from this to save on some duplicated code. You might also be able to do a similar thing for the schema- check out data_source_google_container_cluster.go for an example of a data source that utilitzes the code from the resource really well.
There was a problem hiding this comment.
Fixed, now code looks cleaner - thank you for heads up :)
google/cloudfunctions_helpers.go
Outdated
| @@ -0,0 +1,97 @@ | |||
| package google | |||
There was a problem hiding this comment.
I'd highly recommend taking a look at how we do this sort of thing for other resources- resource_kms_key_ring.go would be a good place to start.
google/cloudfunctions_helpers.go
Outdated
| @@ -0,0 +1,97 @@ | |||
| package google | |||
| } | ||
| } | ||
|
|
||
| func dataSourceGoogleCloudFunctionsFunctionRead(d *schema.ResourceData, meta interface{}) error { |
There was a problem hiding this comment.
Fixed, now code looks cleaner - thank you for heads up :)
| "description": { | ||
| Type: schema.TypeString, | ||
| Optional: true, | ||
| Elem: &schema.Schema{Type: schema.TypeString}, |
| Type: schema.TypeMap, | ||
| Optional: true, | ||
| Elem: &schema.Schema{Type: schema.TypeString}, | ||
| Set: schema.HashString, |
| ConflictsWith: []string{"trigger_http", "trigger_topic"}, | ||
| }, | ||
|
|
||
| "trigger_http": { |
There was a problem hiding this comment.
I was looking into 'gcloud beta functions deploy' CLI and there is trigger-http. So left this field as in gcloud CLI.
|
|
||
| if v, ok := d.GetOk("memory"); ok { | ||
| memory := v.(int) | ||
| if FUNCTION_ALLOWED_MEMORY[memory] != true { |
| return fmt.Errorf("Allowed values for memory are: 128MB, 256MB, 512MB, 1024MB, and 2048MB. "+ | ||
| "Got %d", memory) | ||
| } | ||
| function.AvailableMemoryMb = int64(memory) |
| } | ||
|
|
||
| if !triggHttpOk && !triggTopicOk && !triggBucketOk { | ||
| return fmt.Errorf("One of aguments [trigger_topic, trigger_bucket, trigger_http] is required: " + |
| return err | ||
| } | ||
|
|
||
| name := d.Get("name").(string) |
| if err != nil { | ||
| return fmt.Errorf("Error while updating cloudfunction configuration: %s", err) | ||
| } | ||
| for i := range partialArr { |
google/cloudfunctions_operation.go
Outdated
|
|
||
| func (w *CloudFunctionsOperationWaiter) Conf() *resource.StateChangeConf { | ||
| return &resource.StateChangeConf{ | ||
| Pending: []string{"PENDING", "RUNNING"}, |
There was a problem hiding this comment.
Is "RUNNING" actually a possible state? It looks like you manually set either "PENDING" or "DONE"
google/cloudfunctions_operation.go
Outdated
| var op *cloudfunctions.Operation | ||
| var err error | ||
|
|
||
| op, err = w.Service.Operations.Get(w.Op.Name).Do() |
There was a problem hiding this comment.
you can use := here so you don't have to define the two vars above
| topicName := fmt.Sprintf("tf-test-sub-%s", acctest.RandString(10)) | ||
| zipFilePath, err := createZIParchiveForIndexJs(testHTTPTriggerPath) | ||
| if err != nil { | ||
| t.Errorf(err.Error()) |
There was a problem hiding this comment.
I think these two lines can just be t.Fatal(err)
| } | ||
|
|
||
| resource "google_cloudfunctions_function" "function_http" { | ||
| name = "%s-http" |
There was a problem hiding this comment.
If I understood correctly, I made all ='s in resource have same alignment :)
| } | ||
|
|
||
| resource "google_pubsub_topic" "sub" { | ||
| name = "%s" |
There was a problem hiding this comment.
nit: consistency with tabs vs spaces for indentation (either is fine, just be consistent)
There was a problem hiding this comment.
As per comment above
| switch n { | ||
| case FUNCTION_TRIGGER_HTTP: | ||
| if function.HttpsTrigger == nil { | ||
| return fmt.Errorf("Expected trigger_http to be set") |
There was a problem hiding this comment.
This error gets thrown if field on the returned object isn't set, so instead of trigger_http it would be HttpsTrigger
| return fmt.Errorf("Expected trigger_bucket to be set") | ||
| } | ||
| if strings.Index(function.EventTrigger.EventType, "cloud.storage") == -1 { | ||
| return fmt.Errorf("Expected trigger_bucket to be set") |
There was a problem hiding this comment.
and this one would be something like "Expected cloud.storage EventType" (or even better, "Expected cloud.storage EventType, found %s")
| } | ||
| } | ||
|
|
||
| func createZIParchiveForIndexJs(sourcePath string) (string, error) { |
| resource "google_cloudfunctions_function" "function" { | ||
| name = "%s" | ||
| description = "test function" | ||
| available_memory_mb = 128 |
| } | ||
|
|
||
| resource "google_storage_bucket_object" "archive" { | ||
| name = "index.zip" |
There was a problem hiding this comment.
nit: tabs vs spaces consistency
| addRequiredFieldsToSchema(dsSchema, "name") | ||
|
|
||
| // Set 'Optional' schema elements | ||
| addOptionalFieldsToSchema(dsSchema, "source_archive_bucket", "source_archive_object") |
There was a problem hiding this comment.
Since nothing in the read function actually looks at the value of these in state, I don't think they need to be set to Optional. However, the read function does look at the values of project and region, so you should probably add those here.
| bucketName := fmt.Sprintf("tf-test-bucket-%d", acctest.RandInt()) | ||
| zipFilePath, err := createZIPArchiveForIndexJs(testHTTPTriggerPath) | ||
| if err != nil { | ||
| t.Errorf(err.Error()) |
There was a problem hiding this comment.
you can do fatal here too
| } | ||
| case FUNCTION_TRIGGER_BUCKET: | ||
| if function.EventTrigger == nil { | ||
| return fmt.Errorf("Expected trigger_bucket to be set") |
There was a problem hiding this comment.
trigger_bucket -> EventTrigger
| } | ||
| case FUNCTION_TRIGGER_TOPIC: | ||
| if function.EventTrigger == nil { | ||
| return fmt.Errorf("Expected trigger_bucket to be set") |
There was a problem hiding this comment.
trigger_bucket -> EventTrigger
| source_archive_bucket = "${google_storage_bucket.bucket.name}" | ||
| source_archive_object = "${google_storage_bucket_object.archive.name}" | ||
| trigger_http = true | ||
| timeout = 61 |
There was a problem hiding this comment.
nit: missed one (alignment)
There was a problem hiding this comment.
Fixed, sorry for this - my editor didn't show those tabs...
| source_archive_bucket = "${google_storage_bucket.bucket.name}" | ||
| source_archive_object = "${google_storage_bucket_object.archive.name}" | ||
| trigger_http = true | ||
| timeout = 91 |
There was a problem hiding this comment.
Fixed, sorry for this - my editor didn't show those tabs...
| source_archive_bucket = "${google_storage_bucket.bucket.name}" | ||
| source_archive_object = "${google_storage_bucket_object.archive.name}" | ||
| trigger_topic = "${google_pubsub_topic.sub.name}" | ||
| timeout = 61 |
There was a problem hiding this comment.
Fixed, sorry for this - my editor didn't show those tabs...
| source_archive_bucket = "${google_storage_bucket.bucket.name}" | ||
| source_archive_object = "${google_storage_bucket_object.archive.name}" | ||
| trigger_bucket = "${google_storage_bucket.bucket.name}" | ||
| timeout = 61 |
There was a problem hiding this comment.
Fixed, sorry for this - my editor didn't show those tabs...
| return fmt.Errorf("Function %s doesn't exists.", d.Get("name").(string)) | ||
| } | ||
|
|
||
| if d.HasChange("labels") { |
There was a problem hiding this comment.
Hmm, my guess is that's because the SetPartial wasn't in the right spot before. Can you try again?
|
@danawillow As we discussed I have also changed Update part, so now labels are updated together with other attributes. |
…es function with some values for "timeout" or "available_memory_mb", and then disables those attributes. Terraform plan then gives: No changes. Infrastructure is up-to-date. This is an error. By adding const we would avoid this error.
|
re: your last commit- the reason it was giving that message was because when you removed the default, you added |
| source_archive_bucket = "${google_storage_bucket.bucket.name}" | ||
| source_archive_object = "${google_storage_bucket_object.archive.name}" | ||
| trigger_http = true | ||
| timeout = 61 |
There was a problem hiding this comment.
found some more tabs for you here (and on the same line in the other tests in this file)
| ) | ||
|
|
||
| // Min is 1 second, max is 9 minutes 540 sec | ||
| const FUNCTION_TIMEOUT_MAX = 540 |
There was a problem hiding this comment.
Golang used MixedCase always: https://github.com/golang/go/wiki/CodeReviewComments#mixed-caps
|
I fixed your comments :) Regarding default values. If I omit defaults for timeout and available_memory_mb and create function with terraform apply, then issue 'terraform plan', I would get such output: |
|
Ah ok that's fine then. This looks good, thanks again! |
|
Thank you very much for your review @danawillow :) |
|
Now as code for CloudFunctions is merged into master, what are the next steps? Shall I add some documentation for it? Or is it done by someone else? |
|
Yes please! |
|
This looks super promising. Thanks for the hard work @sarunask and @danawillow :) |
Signed-off-by: Modular Magician <[email protected]>
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks! |
ghost
locked and limited conversation to collaborators
This is PR for #76
Added google_cloudfunctions_function resouse and data providers.