Conversation
Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.79.2 to 1.79.3. - [Release notes](https://github.com/grpc/grpc-go/releases) - [Commits](grpc/grpc-go@v1.79.2...v1.79.3) --- updated-dependencies: - dependency-name: google.golang.org/grpc dependency-version: 1.79.3 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
# Conflicts: # go.mod # go.sum
…p-state-store # Conflicts: # go.mod # go.sum # internal/provider/state_store_http.go # internal/provider/state_store_http_test.go
…rom backend and corresponding tests
| return []func() statestore.StateStore{ | ||
| func() statestore.StateStore { return NewHttpStateStore(p.terraformVersion) }, | ||
| } |
There was a problem hiding this comment.
I'm going to consider this a nit because technically this currently works due to the order of RPC calls + the version data being hardcoded in to Terraform core.
We wouldn't typically recommend providers inject data from configure into these constructor's directly, since Framework doesn't have any control/contract with the provider developer that we are expecting them to use it this way, it's just an internal framework implementation detail.
If in the future we changed when we create state store implementations (as an example), we would expect provider developers to use the recommended provider.ConfigureResponse.StateStoreData -> statestore.InitializeResponse.StateStoreData -> statestore.ConfigureRequest.StateStoreData data flow, so it's possible we could break code like this without knowing. (hyrums law almost always bites us anyways, but if we can avoid it, usually preferable)
| @@ -0,0 +1,5 @@ | |||
| kind: BREAKING CHANGES | |||
| body: 'ProtocolV6: only Terraform 1.0+ is supported for this version of the provider. Earlier versions of Terraform will need to pin the provider version to 3.5.0' | |||
There was a problem hiding this comment.
nit: In relation to this change, looks like we have some other docs to update (maybe more, i didn't do a deep look through the repo) -> https://github.com/hashicorp/terraform-provider-http?tab=readme-ov-file#compatibility
|
|
||
| func (s *httpStateStore) Schema(ctx context.Context, req statestore.SchemaRequest, resp *statestore.SchemaResponse) { | ||
| resp.Schema = schema.Schema{ | ||
| Description: "HTTP state store for managing Terraform state via HTTP endpoints", |
There was a problem hiding this comment.
We'll want to generate the documentation related to this new state store (not yet released in plugin-docs, but it is available on main via hashicorp/terraform-plugin-docs#570).
Once we do that, we should either add a template or update this description to have a little bit more detail about what exactly this state store does and more importantly what it expects the backing APIs to support. The existing HTTP backend is a good place to look to get an idea: https://developer.hashicorp.com/terraform/language/backend/http
In addition to that, we'll want to add some examples, as well as some documentation on migrating from the existing HTTP backend (which we might not be able to write yet until TF core has those exact commands/flags figured out)
| func (s *httpStateStore) Schema(ctx context.Context, req statestore.SchemaRequest, resp *statestore.SchemaResponse) { | ||
| resp.Schema = schema.Schema{ | ||
| Description: "HTTP state store for managing Terraform state via HTTP endpoints", | ||
| Attributes: map[string]schema.Attribute{ |
There was a problem hiding this comment.
Just a general note for all of these attributes, if there is an environment variable default, we should add it to the description (as well as any additional defaults not related to environment variables, see)
| }, | ||
| }, | ||
| "lock_address": schema.StringAttribute{ | ||
| Description: "The address of the HTTP endpoint for state locking. Optional.", |
There was a problem hiding this comment.
Plugin docs will add the optional portion of this message, so we can remove that.
If we want to add any wording, mentioning that locking will be disabled if this isn't provided might be useful 👍🏻
| if req.LockID != "" && (s.client.lockID == "" || s.client.lockID != req.LockID || len(unlockData) == 0) { | ||
| fallbackUnlockInfo := httpLockInfo{ | ||
| ID: req.LockID, | ||
| } | ||
| marshaledUnlockData, err := json.Marshal(fallbackUnlockInfo) | ||
| if err != nil { | ||
| resp.Diagnostics.AddError( | ||
| "Error marshaling unlock info", | ||
| fmt.Sprintf("Error marshaling unlock info to JSON: %s", err), | ||
| ) | ||
| return | ||
| } | ||
| unlockData = marshaledUnlockData | ||
| } |
There was a problem hiding this comment.
This code is very strange and I'm wondering what it's purpose is 🤔
req.LockID should never be empty and if this saved s.client.lockID is not the same as req.LockID, then I would suspect something else has gone wrong. It may be necessary but I would suggest investigating and adding a comment to explain what the goal here is for future maintainers
| return | ||
| } | ||
|
|
||
| httpReq.Header.Set("Content-Type", "application/json") |
There was a problem hiding this comment.
Similar comment to Write/Lock, missing content length + md5 headers
| ) | ||
| } | ||
|
|
||
| func (s *httpStateStore) DeleteState(ctx context.Context, req statestore.DeleteStateRequest, resp *statestore.DeleteStateResponse) { |
There was a problem hiding this comment.
This is a question that might need more investigation, but does DeleteState even get called for a state store like this, which doesn't support multiple workspaces....
I'm not sure if you're allowed to delete the default workspace 🤔 , if so, this might all be dead code 😅
There was a problem hiding this comment.
This is a great chunk of tests that I think covers a good majority of the functionality provided (and more then what was originally tested in TF core!)
The only thing that could potentially be improved here is just some generally refactoring, sharing http test server implementations when possible, etc.
Related Issue
Fixes #
Description
Moving Http from the backend in Terraform core to a state store
Rollback Plan
Changes to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.
No