fix!: Making Cloud Functions ADC Compliant#194
fix!: Making Cloud Functions ADC Compliant#194apeabody merged 3 commits intoGoogleCloudPlatform:mainfrom
Conversation
7108ad5 to
0467c6e
Compare
|
Thanks! After this PR is merged, you will want to open a PR to add: |
Created a PR |
0467c6e to
c0b22f9
Compare
90d39f1 to
90c9629
Compare
|
@apeabody Need your help here. The builds fail with different errors each time w.r.t to the secure functions modules. Are you aware of the modules being used by any teams? @amandakarina Can you help here as well ? |
Hi @prabhu34 - Unfortunately I'm not familiar, but the current build failed due to:
For the later, assuming it's a random conflict, I recommend adding: |
|
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days |
d6f392a to
ecaa891
Compare
|
Picking up this PR again, trying to resolve the build errors |
3be28a9 to
09d7fe2
Compare
09d7fe2 to
71675b7
Compare
Hi @anshukaira You renamed the project to |
71675b7 to
69075bc
Compare
|
hi @apeabody |
| module "cloud_functions2" { | ||
| source = "GoogleCloudPlatform/cloud-functions/google" | ||
| version = "~> 0.6" | ||
| source = "../../" |
There was a problem hiding this comment.
This is automatically rewritten during lint/tests, so the standard is to use a version in the committed version for customers.
| source = "../../" | |
| source = "GoogleCloudPlatform/cloud-functions/google" | |
| version = "~> 0.7" |
There was a problem hiding this comment.
For future fix: update to use "released" version and depend on module swapper for lint and & tests
| module "cloud_functions2" { | ||
| source = "GoogleCloudPlatform/cloud-functions/google" | ||
| version = "~> 0.6" | ||
| source = "../../" |
There was a problem hiding this comment.
| source = "../../" | |
| source = "GoogleCloudPlatform/cloud-functions/google" | |
| version = "~> 0.7" |
There was a problem hiding this comment.
Update:
Tried doing this in this test PR - https://github.com/GoogleCloudPlatform/terraform-google-cloud-functions/pull/205/checks?check_run_id=61342778210
and i am seeing failures in test as well as lint. scheduled a call on 26/01 to resolve this issue.
There was a problem hiding this comment.
For future fix: update to use "released" version and depend on module swapper for lint and & tests
| @@ -35,9 +35,9 @@ module "secure_harness" { | |||
| version = "~> 0.21.5" | |||
There was a problem hiding this comment.
For future change: It's bump the version constraint to "~> 0.23"
There was a problem hiding this comment.
| @@ -36,7 +36,7 @@ module "secure_harness" { | |||
| version = "~> 0.21.5" | |||
There was a problem hiding this comment.
For future change: It's bump the version constraint to "~> 0.23"
| @@ -39,8 +39,8 @@ module "secure_harness" { | |||
| version = "~> 0.21.5" | |||
There was a problem hiding this comment.
For future change: It's bump the version constraint to "~> 0.23"
apeabody
left a comment
There was a problem hiding this comment.
Thanks @anshukaira!
Approved with the notes to please follow-up in a future fix PR.
|
Created a followup PR for this -- #212 |
| locals { | ||
| # If var.location is set, use it. Otherwise, use var.function_location. | ||
| # This makes the change backward-compatible. | ||
| effective_location = coalesce(var.location, var.function_location) | ||
| } | ||
|
|
There was a problem hiding this comment.
This change is not backwards compatible because var.location has no default. So if this module 'A' is consumed in another module 'B', this change has broken module 'B' until it is updated.
ref: terraform-google-modules/terraform-example-foundation#1458
There was a problem hiding this comment.
we have created a new major version by using ! as this is a breaking change. So, it should not be breaking other modules if they are using the old major version of cloud functions.
There was a problem hiding this comment.
@krprabhat-eng but the major version was released as a minor version, 0.8.0.
Terraform's ~> 0.6 operator will resolve to 0.8, as in the linked PR
There was a problem hiding this comment.
@anshukaira can we approve this & create a new major version for our use case.
@kevcube When we are releasing a new major version, can we simply rename function_location to location ?
There was a problem hiding this comment.
Yes, removing function_location in favor of location in a new major release is fine @krprabhat-eng
Making Cloud Functions ADC Compliant