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

Etcd remote state backend #3487

Merged
merged 2 commits into from
Oct 20, 2015
Merged

Etcd remote state backend #3487

merged 2 commits into from
Oct 20, 2015

Conversation

edef1c
Copy link
Contributor

@edef1c edef1c commented Oct 12, 2015

I'd like to get this merged, it's incredibly useful for people already running CoreOS clusters
who still require some form of self-hosted infrastructure management.

The etcd project unfortunately doesn't provide a public demo server,
so the tests for this backend require providing an etcd endpoint.

@edef1c
Copy link
Contributor Author

edef1c commented Oct 12, 2015

It should probably be noted that I'm base64-encoding the state, since etcd doesn't currently support binary values.

@edef1c
Copy link
Contributor Author

edef1c commented Oct 12, 2015

I'm not sure if it's actually necessary to encode the values, given that they appear to be just JSON in practice.
The interface allows for arbitrary byte strings, however.

@apparentlymart
Copy link
Contributor

I initially made the S3 backend use a generic MIME type because the interface allows arbitrary bytes, but we recently changed it to application/json because practically that is what it is.

Assuming you're going to get a valid JSON payload seems reasonable to me, and thus assuming that it will be UTF-8 encoded text.

@edef1c
Copy link
Contributor Author

edef1c commented Oct 14, 2015

Okay, updated it to just deal with strings — I'm in favour of keeping things simple [=

@apparentlymart
Copy link
Contributor

This looks great to me. Thanks!

Would you mind adding a section to the remote config docs describing how to use this? You could just use one of the others as an example. I think once there are docs here this will be good to go!

* Etcd - Stores the state in etcd at a given path.
Requires the `path` and `endpoints` variables. The `username` and `password`
variables can optionally be provided. `endpoints` is assumed to be a
comma-separated list of etcd endpoints.
Copy link
Contributor

Choose a reason for hiding this comment

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

Reading these docs made me spot something I didn't catch the first time around, because I tested with only one endpoint: isn't endpoints a list of URLs? It seems strange to me to delimit a list of URLs with commas because commas are valid within URLs.

I suppose practically-speaking an etcd endpoint URL is only going to contain a scheme, host and port and so there aren't any opportunities for commas there, but could it instead be a space-separated list of URLs so that e.g. auto-linkification algorithms used on various web forums won't be tempted to consider the whole list of endpoints to be a single, long URL?

For example, Github seems to exhibit the behavior I'm talking about:

-config="endpoints=http://127.0.0.1:1234/,http://127.0.0.1:4321/"

Not a huge deal, but something I could see being confusing to people trying to discuss Terraform.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered spaces, I went for commas mainly because handling arguments with spaces in them in shell scripts is something people tend to mess up. Commas seemed rare enough in URLs for this to be worth it. I'm going to change this to spaces, though — two votes for spaces, and only one for commas.

@apparentlymart
Copy link
Contributor

Sorry @nathan7 one more quick thing I didn't catch the first time. I know it's a bit of trivia but these things can be hard to change later so I'm interested to hear what you think.

@apparentlymart
Copy link
Contributor

Awesome, thanks! Sorry for the messing around. 🚢

apparentlymart added a commit that referenced this pull request Oct 20, 2015
Etcd remote state backend
@apparentlymart apparentlymart merged commit 484d6f8 into hashicorp:master Oct 20, 2015
@apparentlymart
Copy link
Contributor

When I merged this there was a build failure relating to packngo which seems to be in the packet provider and not related to this change. If I'm wrong please let me know and I'll clean it up.

@edef1c edef1c deleted the etcd branch October 21, 2015 00:55
@ghost
Copy link

ghost commented Apr 30, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants