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

Implemented new http data-source to deal with external HTTP endpoints #14270

Merged
merged 1 commit into from
May 9, 2017

Conversation

korotovsky
Copy link
Contributor

Hi,

First, thanks for creating the Terraform tool!

I would like to purpose next enhancement for Terraform. Sometimes it's not possible to re-create and provision resources from scratch and for that purpose we use null_recourse + triggers combination to run provisioners. But also we have some additional external metadata in our centralized registry and when data is changed there, then on next run Terraform will re-run provisioners again.

Here is an example:

resource "null_resource" "service-provisioner" {
  triggers {
    instance_count = "${var.some_var}"
    // ..
    version = "${url("https://gist.githubusercontent.com/anonymous/a738a343ef2a44c1bafe69b72b9a69dc/raw/0aa990256dfc682ade55c9f2614ff20689512d88/1.0.0")}"
  }

  provisioner "remote-exec" {
    connection {
      // ...
    }

    inline = [
      "echo 'OK!'"
    ]
  }

To achieve this behavior I've added the url(string) interpolation function. I've also added some tests, docs have been updated.

Thanks.

@apparentlymart
Copy link
Contributor

Hi @korotovsky! Thanks for working on this.

Your use-case here makes a lot of sense to me, and I agree that having a simple way to retrieve data over HTTP would be useful.

Previous experience with the file interpolation function makes me hesitant to include this as an interpolation function, however. Interpolation functions have the problem that they don't evaluate at a single, predictable time in the lifecycle (e.g. they may be evaluated once during validation, again during planning). They also can't safely interact with resources that are created elsewhere in the configuration, since they don't participate in the dependency graph.

I would suggest to instead address this use-case via a data source:

data "http" "example" {
  url = "https://gist.githubusercontent.com/anonymous/a738a343ef2a44c1bafe69b72b9a69dc/raw/0aa990256dfc682ade55c9f2614ff20689512d88/1.0.0"
}

resource "null_resource" "service-provisioner" {
  triggers {
    instance_count = "${var.some_var}"
    version        = "${data.http.example.body}"
    # ...
  }

  provisioner "remote-exec" {
    # ...
  }
}

While this is definitely more verbose, it has the benefit that the HTTP get participates in the dependency graph along with everything else, so it will get retrieved just once when refreshing the data sources and the timing of that GET can be influenced by resource dependencies.

I appreciate the work you already put into this so I don't mean to suggest by this that I expect you to start over and implement this data source instead. However, if you are interested in working on it, I would suggest making it behave similarly to the existing aws_s3_bucket_object data source (which is, after all, just a specialized HTTP GET with AWS credentials), in particular only populating the body attribute if the Content-Type looks like something text-friendly, since Terraform's internals don't guarantee safe passage for raw binary data.

It would be totally reasonable for you to say that you've got no further time to spend on this: no pressure whatsoever. If this rework is not something you can take on, please let me know and I can try to find a place for this on my personal to-do list, since I agree it's a good thing to support.

Thanks again!

@apparentlymart apparentlymart self-requested a review May 7, 2017 17:01
@korotovsky korotovsky changed the title Added url(string) interpolation function for reading remote content. Implemented new http data-source to deal with external HTTP endpoints May 8, 2017
@korotovsky
Copy link
Contributor Author

Hi @apparentlymart,

All done (I hope). Some notes:

I copied the function isContentTypeAllowed() from the existing aws_s3_bucket_object data source to make less impact on the whole Terraform project by my PR. So at the end it introduces only new http provider and has no refactoring in rest parts. I believe, it should be done in the different issue/PR.

Thanks.

@apparentlymart
Copy link
Contributor

Thanks for this new version, @korotovsky!

On a first pass it all looked good to me. I'll take another closer look soon when I have some more time to review/test, and then we can get this merged.

@apparentlymart apparentlymart merged commit ace0456 into hashicorp:master May 9, 2017
@apparentlymart
Copy link
Contributor

Thanks again, @korotovsky!

Before I merged I made some small adjustments to the documentation, and I renamed the headers attribute to request_headers in case we want to in future add an attribute response_headers to expose the response headers.

Awesome work here! I especially liked how the unit tests bring their own temporary web server so that they can run in a self-contained way.

@apparentlymart apparentlymart removed their request for review May 9, 2017 00:40
@ghost
Copy link

ghost commented Apr 12, 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 12, 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.

2 participants