Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create dataflow flex template module #168

Merged

Conversation

daniel-cit
Copy link
Collaborator

@daniel-cit daniel-cit commented Oct 6, 2021

Fixes #163

It's a good idea to open an issue first for discussion.

  • Tests pass
  • Appropriate changes to README are included in PR
  • replace resource calls in examples

Copy link
Member

@erlanderlo erlanderlo left a comment

Choose a reason for hiding this comment

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

Wow! I like how you approached this and can be quite helpful.

modules/dataflow-flex-job/main.tf Show resolved Hide resolved
modules/dataflow-flex-job/main.tf Show resolved Hide resolved
modules/dataflow-flex-job/main.tf Show resolved Hide resolved
@daniel-cit daniel-cit marked this pull request as ready for review October 8, 2021 17:21
@daniel-cit daniel-cit requested a review from erlanderlo October 11, 2021 12:07
Copy link
Member

@erlanderlo erlanderlo left a comment

Choose a reason for hiding this comment

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

LGTM on the examples using the flex-template module.

Copy link
Member

@bharathkkb bharathkkb 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 PR. I was wondering if we can upstream necessary changes to https://github.com/terraform-google-modules/terraform-google-dataflow and use that instead of having maintain this in two places?

modules/dataflow-flex-job/outputs.tf Outdated Show resolved Hide resolved
modules/dataflow-flex-job/README.md Outdated Show resolved Hide resolved
@daniel-cit
Copy link
Collaborator Author

daniel-cit commented Oct 11, 2021

Thanks for the PR. I was wondering if we can upstream necessary changes to https://github.com/terraform-google-modules/terraform-google-dataflow and use that instead of having maintain this in two places?

I am not sure on upstreaming the code because we are a bit more opinionated about some items:

  • we don't have the default network as a valid default value, we require a explicit subnetwork self link
  • we require the user to provide a KMS key
  • we require the user to provide a dataflow controller service account
  • we require the to provide a staging location
  • we have explicit support fort network tags

@bharathkkb @erlanderlo

@erlanderlo
Copy link
Member

It's likely a future item to move it upstream, and for now we wanted to modularize things first and observe how the module is used with these very specific items that were mentioned.

Copy link
Member

@erlanderlo erlanderlo left a comment

Choose a reason for hiding this comment

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

LGTM; I'm guessing the tests are decoupled because we will be added using the newer go framework

@bharathkkb
Copy link
Member

@daniel-cit @erlanderlo yup if we have opinionated defaults lets use this as a thin wrapper around the upstream module to enforce specific patterns. We can do this as a follow up. Ideally we want the upstream module to support security focused usage patterns similar to safer cluster wrapping main GKE module.

@daniel-cit
Copy link
Collaborator Author

@daniel-cit @erlanderlo yup if we have opinionated defaults lets use this as a thin wrapper around the upstream module to enforce specific patterns. We can do this as a follow up. Ideally we want the upstream module to support security focused usage patterns similar to safer cluster wrapping main GKE module.

Opened issue terraform-google-modules/terraform-google-dataflow#32 in the dataflow module to track this

@bharathkkb bharathkkb merged commit c0398be into GoogleCloudPlatform:main Oct 13, 2021
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.

dataflow flex template module
3 participants