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 lxd_cached_image resource #43

Merged
merged 2 commits into from
Apr 25, 2017
Merged

Add lxd_cached_image resource #43

merged 2 commits into from
Apr 25, 2017

Conversation

sl1pm4t
Copy link
Collaborator

@sl1pm4t sl1pm4t commented Apr 23, 2017

This adds the lxd_cached_image resource. It can used to manage local
copies of remote images, set aliases etc.

I've named it lxd_cached_image to distinguish it from a future
resource I'm planning to create, that will allow users to create an LXD
image from a tarball.

This adds the `lxd_cached_image` resource. It can used to manage local
copies of remote images, set aliases etc.

I've named it `lxd_cached_image` to distinguish it from a future
resource I'm planning to create, that will allow users to create an LXD
image from a tarball.
@sl1pm4t sl1pm4t requested a review from jtopjian April 23, 2017 08:27
@jtopjian
Copy link
Collaborator

Nice resource :)

With regard to aliases, copy_aliases, and copied_aliases, if I'm understanding it correctly, this is a common pattern emerging in some Terraform resources. I recently went through this with two OpenStack resources. The recommended approach was to treat one as an "input" and one as a computed read-only "output". So you could have aliases as the input and all_aliases as the output. (all_ being the suggested prefix).

In the Read function, you would no longer do a d.Set("aliases", x) since the user's configuration will be the source of truth. all_aliases, on the other hand, will be the API's source of truth. Depending on the results, the value of all_aliases might just equal aliases, or a combination of aliases and upstream-specified aliases.

Makes it a lot easier to handle these situations :)

And I think you could remove the copy_aliases attribute by doing this, too.

Thoughts?

@sl1pm4t
Copy link
Collaborator Author

sl1pm4t commented Apr 23, 2017

Yes, the aliases are a bit of a PITA - and will probably go mostly unused. Especially if all the resources are managed with Terraform, there shouldn't be much need for the aliases.
However, having already coded up handling of the aliases I might as well keep it in place.
If I fall back to a aliases input that doesn't get updated on Read, then it would make it impossible to ever add or remove aliases right?!

copied_aliases is slightly different to all_aliases, in that it only includes the aliases that were defined in the remote LXD server at the time the image copy was created. If it were possible, I would make it an internal hidden attribute used only for the purposes of suppressing diffs.

I could also add all_aliases output that you mention, that reflects the merged set of aliases.

@jtopjian
Copy link
Collaborator

OK, I understand. The key here is the ability to opt-in to setting aliases. If it was mandated that downloaded/cached images received the upstream aliases, then the alias, all_aliases combo would make sense.

Give me a day or so to test this and poke at it. :)

@jtopjian
Copy link
Collaborator

This looks good to me -- everything does make sense.

I opened #44 which uses Terraform core's built-in set functions, but really it's six one way, half a dozen the other. I think playing around with the scenario of the aliases/copied_aliases can go on all day, so feel free to discard it. :)

@sl1pm4t sl1pm4t merged commit e80bf3e into master Apr 25, 2017
@sl1pm4t sl1pm4t deleted the lxd-cached-image-rebase branch April 25, 2017 07:51
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

2 participants