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

Container file resource #116

Merged
merged 4 commits into from
Dec 17, 2017

Conversation

jtopjian
Copy link
Collaborator

This commit adds the lxd_container_file resource which can be used
to manage individual files in a container.

This depends on #112

I can rebase afterwards or all 3 commits can be merged here.

Can also add docs once #114 is added.

@mjrider
Copy link
Contributor

mjrider commented Nov 27, 2017

I'm wondering why you want to move the file subresource to a seperate resource
from the following 2 examples, i think the second is the most readable, and has the least surprises, so which scenario are you trying to cover here?

resource "lxd_container" "container1" {
   name = "foobar"
   image = "images:alpine/3.5"
   profiles = ["default"]
 }
 
 resource "lxd_container_file" "file1" {
   container_name = "${lxd_container.container1.name}"
   remote  = "${lxd_container.container1.remote}"
   target_file = "/foo/bar.txt"
   content = "Hello, World!\n"
   create_directories = true
 }

vs

resource "lxd_container" "container1" {
   name = "foobar"
   image = "images:alpine/3.5/amd64"
   profiles = ["default"]
   file {
     target_file = "/foo/bar.txt"
     content = "Hello, World!\n"
     create_directories = true
   }
 }

@jtopjian
Copy link
Collaborator Author

jtopjian commented Nov 27, 2017

@mjrider See my comments in #112

Additionally, when a resource has "sub-resources" that tend to have a one-to-many relationship, they always work better as separate resources in the long term. For example, by having a separate file resource, the file can be treated as a first-class resource and have dedicated count and depends_on arguments.

@sl1pm4t
Copy link
Collaborator

sl1pm4t commented Dec 1, 2017

What would be the problem you see with keeping the original embedded file sub resource along side the standalone resource?
I know it would some caveats ( the two resources compete and TF will see a diff on each run ) but it seems to be a commonly accepted pattern in other providers. e.g google_container_cluster that includes a node_pool sub resource vs the standalone google_container_node_pool resource.

This commit adds the lxd_container_file resource which can be used
to manage individual files in a container.
@jtopjian
Copy link
Collaborator Author

jtopjian commented Dec 2, 2017

Rebased and docs added.

@mjrider
Copy link
Contributor

mjrider commented Dec 2, 2017

@jtopjian what do you think about sl1pm4t suggestion for keeping the subresource version , so users can have a choice in what they need/want

@jtopjian
Copy link
Collaborator Author

jtopjian commented Dec 2, 2017

@mjrider That was my intention all along 😄

@shantanugadgil
Copy link
Contributor

FWIW, I don't mind having only a separate resource.

sl1pm4t
sl1pm4t previously approved these changes Dec 3, 2017
Copy link
Collaborator

@sl1pm4t sl1pm4t left a comment

Choose a reason for hiding this comment

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

LGTM with one tiny typo found.

lxd/shared.go Outdated
func containerDeleteFile(server lxd.ContainerServer, container string, targetFile string) error {
targetFile, err := filepath.Abs(targetFile)
if err != nil {
return fmt.Errorf("Could not santize destination target %s", targetFile)
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo - santize -> sanitize

@sl1pm4t
Copy link
Collaborator

sl1pm4t commented Dec 3, 2017

That was my intention all along 😄

Sorry for the confusion, when I read through the PR comments on my phone I somehow got the impression you were removing the embedded resource. Might be because there was another PR for deprecated a resource, and I confused the two.

Anyway it looks good to me.

@jtopjian
Copy link
Collaborator Author

jtopjian commented Dec 4, 2017

No problem at all!

I detailed my plans for this in #112. I should have added those notes here as well, so it's my fault for the confusion.

Thank you for catching the typo.

@sl1pm4t sl1pm4t merged commit 27cc371 into terraform-lxd:master Dec 17, 2017
@jtopjian jtopjian deleted the container-file-resource branch December 13, 2020 01:45
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.

None yet

4 participants