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

feat: add support for read_data #252

Merged
merged 3 commits into from
Mar 1, 2024

Conversation

detvdl
Copy link
Contributor

@detvdl detvdl commented Dec 12, 2023

Similar to #182, added support for an argument read_data in order to support not-so-RESTful API designs.

Case study is Scalyr/DataSet, whose API for fetching configuration files is implement as a set of POST requests, with the hard requirement that there is a request body (even if it is empty):

Tested against this API with some logParser objects, and this helps to support a use case such as:

provider "restapi" {
  uri = "https://app.scalyr.com/api"
  headers = {
    "Authorization" = "Bearer ${local.api_token}"
    "Content-Type"  = "application/json"
  }
  debug                = false
  write_returns_object = false
}

resource "restapi_object" "scalyr_parsers" {
  for_each      = local.parsers
  object_id     = "/logParsers/${each.key}"
  path          = "/"
  create_method = "POST"
  update_method = "POST"
  read_method   = "POST"
  read_path     = "/getFile"
  read_data = jsonencode({
    path = "/logParsers/${each.key}"
  })
  create_path = "/putFile"
  data = jsonencode({
    path    = "/logParsers/${each.key}"
    content = each.value
  })
}

@detvdl
Copy link
Contributor Author

detvdl commented Dec 12, 2023

Before I move ahead with also attempting to support this in data sources, is this something that would be useful?

Presently, I have no need to support this use-case from a data source perspective, but wouldn't mind adding it if this is deemed useful in any way

@detvdl
Copy link
Contributor Author

detvdl commented Dec 12, 2023

Duplicate of #177 it seems, any advice on how to move forward with this?

It seems to highlight that there is a need for such a field, but I'm open to suggestions

@DRuggeri
Copy link
Member

DRuggeri commented Mar 1, 2024

Thanks for the PR @detvdl !
I do think this makes sense and it would be generally useful, so ought to be brought into the provider. Thanks for including the test cases - this is g2g, IMO. I know you opened this a bit ago, but would you like to also add the Datasource support now before merge?

Also, thanks for the patience! As mentioned in the readme, we keep the provider up to date but features are ssssllllloooooowwwwww.

@detvdl
Copy link
Contributor Author

detvdl commented Mar 1, 2024

Hi @DRuggeri,

I don't have a use-case for implementing this in data sources currently, and I think it might need a bit more of a re-work of the data source handling in general.
The APIs for the Resource and Data source are quite different right now.

If it's okay, I would prefer to look at that separately, at a later date. If this is g2g for you, then feel free to merge and release as you see fit 🙂

@DRuggeri DRuggeri merged commit 8c887f3 into Mastercard:master Mar 1, 2024
@DRuggeri
Copy link
Member

DRuggeri commented Mar 1, 2024

Right on - this will be released in 1.19.0 which is building now!

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.

2 participants