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

add support for object upload to gcs module #1584

Merged

Conversation

ehorning
Copy link
Contributor

Allow users to upload files to GCS as part of GCS module

@juliocc
Copy link
Collaborator

juliocc commented Aug 11, 2023

Hi @ehorning,

I was discussing with @ludoo and while we're a bit ambivalent about this PR, we're willing to give it a shot as long as others don't have any strong objections (@thinhha, @lcaggio what do you think about this PR).

That being said, the google_storage_bucket_object resource has a lot more options. If we're going to allow the module to create objects, we should expose all/most of those features. Otherwise, we should just recommend to use the resource directly.

@ehorning
Copy link
Contributor Author

Hi @ehorning,

I was discussing with @ludoo and while we're a bit ambivalent about this PR, we're willing to give it a shot as long as others don't have any strong objections (@thinhha, @lcaggio what do you think about this PR).

That being said, the google_storage_bucket_object resource has a lot more options. If we're going to allow the module to create objects, we should expose all/most of those features. Otherwise, we should just recommend to use the resource directly.

Is the ambivalence just around the question of whether or not users should be able to upload objects through terraform? I can see how there might be an argument against this in many cases due to the possibility that it might increase the likelihood of sensitive information getting stored in terraform source control. The use case I encountered was around streamlining the upload of sample data via terraform, but I would buy the argument that this might not be a great idea in general so if others think there isn't much need for this functionality, I'm ok with discarding this PR.

If others do think this could be valuable in certain use cases, I will add those other options.

@ludoo
Copy link
Collaborator

ludoo commented Aug 11, 2023

It's more due to our general approach as infra/net people that modules should be used to create the underlying infra, and we don't really care about workload specifities. Which could be just selective blindness from our side, so that isn't really an issue especially once a few of the data people contributing to this repo chime, as we suspect that supporting objects will make a lot of sense to them.

The real blocker is doing it properly by supporting all or most of the attributes exposed by the resource. Do you feel like giving it a try? We can help sketch out the variable interface if you want.

@ehorning
Copy link
Contributor Author

It's more due to our general approach as infra/net people that modules should be used to create the underlying infra, and we don't really care about workload specifities. Which could be just selective blindness from our side, so that isn't really an issue especially once a few of the data people contributing to this repo chime, as we suspect that supporting objects will make a lot of sense to them.

The real blocker is doing it properly by supporting all or most of the attributes exposed by the resource. Do you feel like giving it a try? We can help sketch out the variable interface if you want.

Sure I can do that. Point about workloads makes sense to me as someone who largely works on infra as well so curious what thoughts others have.

@lcaggio
Copy link
Collaborator

lcaggio commented Aug 22, 2023

I see the need to have the ability to upload files for a demo/example purpose.

What about having a separate module to upload files into existing bucket? this would make it more flexible.

Note on writing blueprints with demo data: I would suggest to have the demo data deployed as optional, controlled by a bool variable (e.g. deploy_demo_data).

@ludoo
Copy link
Collaborator

ludoo commented Aug 22, 2023

I see the need to have the ability to upload files for a demo/example purpose.

What about having a separate module to upload files into existing bucket? this would make it more flexible.

Note on writing blueprints with demo data: I would suggest to have the demo data deployed as optional, controlled by a bool variable (e.g. deploy_demo_data).

I would keep all part of the same module.

Copy link
Collaborator

@juliocc juliocc 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 changes! This LGTM now.

One more thing to consider, but this can be a separate PR: an "objects" output with the created object details (self_link, name, hash, etc)

modules/gcs/variables.tf Outdated Show resolved Hide resolved
Copy link
Collaborator

@juliocc juliocc left a comment

Choose a reason for hiding this comment

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

LGTM now.

modules/gcs/variables.tf Outdated Show resolved Hide resolved
@juliocc juliocc enabled auto-merge August 22, 2023 16:42
@juliocc juliocc merged commit 927c04a into GoogleCloudPlatform:master Aug 22, 2023
@ehorning ehorning deleted the ehorning/support-gcs-object-upload branch August 22, 2023 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants