Skip to content

Conversation

tardigrde
Copy link
Contributor

@tardigrde tardigrde commented Apr 21, 2023

Variables added:

  • docker_registry
  • docker_repository
  • kms_key_name

This enables storing Cloud Build container in Artifact Registry

See https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/cloudfunctions_function#docker_registry

@tardigrde tardigrde requested a review from a team as a code owner April 21, 2023 22:17
@tardigrde
Copy link
Contributor Author

tardigrde commented Apr 21, 2023

Hi! Could you please look at this merge request? I know I created and closed some PRs with the same scope, those were problematic ecause of failing CLAs. Now the commit is with an email address that signed a CLA.

Happy to improve the PR if needed. Let me know if I have to do anything else.

The important thing for me was that I can use Artifact Registry for storing Cloud Build image. I think no default functionality was changed.

Note: I also would like to update https://github.com/terraform-google-modules/terraform-google-scheduled-function with this same functionality.

Can you please run /gcbrun? I think I do not have the rights to do it.

@tardigrde tardigrde changed the title feat: Add variables and use them in google_cloudfunctions_function resource feat: Add docker_registry variables and use them in google_cloudfunctions_function resource Apr 21, 2023
@apeabody
Copy link
Contributor

/gcbrun

@apeabody
Copy link
Contributor

Thanks for the contribution @tardigrde - Relevant output from the LINT tests:

Checking for documentation generation
diff -r '--exclude=.terraform' '--exclude=.kitchen' '--exclude=autogen' '--exclude=*.tfvars' '--exclude=*metadata.yaml' /workspace/README.md /tmp/tmp.7eUSZEwJ41/generate_docs/workspace/README.md
64c64
< | docker\_repository | User managed repository created in Artifact Registry optionally with a customer managed encryption key. If specified, deployments will use Artifact Registry. | `string` | n/a | yes |
---
> | docker\_repository | User managed repository created in Artifact Registry optionally with a customer managed encryption key. If specified, deployments will use Artifact Registry. | `string` | `""` | no |
71c71
< | kms\_key\_name | Resource name of a KMS crypto key (managed by the user) used to encrypt/decrypt function resources. | `string` | n/a | yes |
---
> | kms\_key\_name | Resource name of a KMS crypto key (managed by the user) used to encrypt/decrypt function resources. | `string` | `""` | no |
Error: Documentation generation has not been run, please run the
'make docker_generate_docs' command and commit the above changes.
Checking for trailing whitespace
Checking for missing newline at end of file
Error: No newline at end of file ./variables.tf

Fix variables in README
@tardigrde
Copy link
Contributor Author

Hey! I fixed the linting problems, can you please do gcbrun again?

See commit: 4ee3f29

@apeabody
Copy link
Contributor

/gcbrun

@tardigrde
Copy link
Contributor Author

Hi @apeabody ! The checks have been passed, now I tried to updae the branch, but again a gcbrun is needed. Can you please review my changes and merge if no problems found. Thanks!

@apeabody
Copy link
Contributor

apeabody commented May 4, 2023

/gcbrun

Copy link
Contributor

@apeabody apeabody left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @tardigrde!

I've included a few comments below, and additionaly it doesn't appear all these fields are present in TPG 4.11, so https://github.com/terraform-google-modules/terraform-google-event-function/blob/master/versions.tf#L23 will need to be bumped to an appropriate minimum version.

Thanks!

@tardigrde
Copy link
Contributor Author

Thanks for the heads up! I'm still learning..

Bump google provider min version to v4.23
v4.22 does not have the parameters yet, but v4.23 does have them, see versioned doc: https://registry.terraform.io/providers/hashicorp/google/4.23.0/docs/resources/cloudfunctions_function

@tardigrde tardigrde requested a review from apeabody May 12, 2023 12:46
@alexrohv
Copy link

Ha! I should probably pay closer attention to open PRs. #165 is pretty much a copy of this, as I need this feature as well.

@apeabody @tardigrde Any way we can move it forward?

@apeabody
Copy link
Contributor

/gcbrun

@apeabody
Copy link
Contributor

Ha! I should probably pay closer attention to open PRs. #165 is pretty much a copy of this, as I need this feature as well.

@apeabody @tardigrde Any way we can move it forward?

Triggered the CI

@apeabody
Copy link
Contributor

/gcbrun

@apeabody
Copy link
Contributor

Hi @tardigrde - When you have a chance please run make docker_generate_docs, thanks!

@apeabody apeabody changed the title feat: Add docker_registry variables and use them in google_cloudfunctions_function resource feat(TPG >=4.23)!: Add docker_registry variables and use them in google_cloudfunctions_function resource May 20, 2023
@tardigrde
Copy link
Contributor Author

Updated! Anything else?

@apeabody
Copy link
Contributor

/gcbrun

Copy link
Contributor

@apeabody apeabody left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @tardigrde!

@apeabody apeabody merged commit 12beb96 into terraform-google-modules:master May 23, 2023
@tardigrde
Copy link
Contributor Author

@apeabody would you be so kind to make a release? I'd like to get this PR merged too, and for that I need a release in this repo (it's a dependency to scheduled-function module)

@apeabody
Copy link
Contributor

@apeabody would you be so kind to make a release? I'd like to get this PR merged too, and for that I need a release in this repo (it's a dependency to scheduled-function module)

Hi @tardigrde - Sure, please track the release at #167

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants