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 initial support for reference tables #196

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

obs-gh-abhinavpappu
Copy link
Collaborator

  • move current REST client (only used for login) from client/internal/customer to client/rest
  • add helper functions for POST, GET, PUT, PATCH, and DELETE
    • return error type including status code, allowing resourceReferenceTableRead to determine whether the reference table exists (code 404)
  • add types and functions to call reference table API - will be mostly generated in a future PR
  • add new resource and data source observe_reference_table
    • To avoid storing the entire (potentially very large) file in the state, as well as needing to fetch the entire file on every plan/refresh/apply to check for changes, we use a checksum field to detect changes instead. This checksum should always be set to filemd5("<path_to_source_file>"). The API returns the current remote checksum, which is compared against the local one.
    • There are two ways to update a reference table, PUT and PATCH. PUT will replace everything with what's provided, and PATCH will update only the metadata fields that are provided in the request, leaving the others as is. PATCH can't be used to update the file itself.
  • add basic tests using some test csv files added to observe/testdata

In future PRs, will:

  • Add support for the schema field. This isn't returned by the API, but can be retrieved from the underlying dataset through a GraphQL request.
  • Generate types (and functions?) from the OpenAPI spec using openapi-generator
  • A few other minor changes after API is updated, indicated by TODO comments

"github.com/observeinc/terraform-provider-observe/client/oid"
)

// TODO: generate types from OpenAPI spec
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect most of this file can be auto-generated from the spec once we settle on the mechanics

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, got something working for generation and this file can be removed entirely. But requires some OpenAPI spec changes that are currently still in progress, so will add in a separate PR once those are in

docs/resources/reference_table.md Outdated Show resolved Hide resolved
docs/data-sources/reference_table.md Show resolved Hide resolved
observe/resource_reference_table.go Show resolved Hide resolved
diags = append(diags, diag.FromErr(err)...)
}

// TODO: add "primary_key" and "label_field" once supported
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if you meant to add it in this PR as we do support it? Or do we need support on the api front for this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Need API support. Currently it doesn't return primary_key or label_field in the response, but they will be added soon. So currently, we're unable to detect changes to those fields made outside of terraform, but it should work fine as long as all changes are made through terraform. Don't think this is a blocker since there isn't UI support for editing reference tables, so the only way to change those fields outside of terraform is through the API (which is unlikely to happen for a terraform-managed resource).

observe/resource_reference_table.go Show resolved Hide resolved
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