feat: Add workspace_name field in stream_connection resource and datasource

[kpatel71716]

Description

JIRA: CLOUDP-339895, following the plan discussed with APIx team in CLOUDP-336156

As part of an ongoing effort to rename stream instances to stream workspaces, we will support a workspace_name field for the stream_connection resource and datasource that will function identically to 1instance_name`. To avoid confusion, only one of instance_name or workspace name will be allowed to be set

In a future major version of terraform, we will remove the instance_name field

Link to any related issue(s):

Type of change:

• Bug fix (non-breaking change which fixes an issue). Please, add the "bug" label to the PR.
• New feature (non-breaking change which adds functionality). Please, add the "enhancement" label to the PR. A migration guide must be created or updated if the new feature will go in a major version.
• Breaking change (fix or feature that would cause existing functionality to not work as expected). Please, add the "breaking change" label to the PR. A migration guide must be created or updated.
• This change requires a documentation update
• Documentation fix/enhancement

Required Checklist:

• I have signed the MongoDB CLA
• I have read the contributing guides
• I have checked that this change does not generate any credentials and that they are NOT accidentally logged anywhere.
• I have added tests that prove my fix is effective or that my feature works per HashiCorp requirements
• I have added any necessary documentation (if appropriate)
• I have run make fmt and formatted my code
• If changes include deprecations or removals I have added appropriate changelog entries.
• If changes include removal or addition of 3rd party GitHub actions, I updated our internal document. Reach out to the APIx Integration slack channel to get access to the internal document.

Further comments

[github-actions]

APIx bot: a message has been sent to Docs Slack channel

[marcosuma]

what do we mean here with overridden?

[kpatel71716]

This was unnecessary. I removed it

[kpatel71716]

Actually looks like we still need this so that terraform can use the workspace_name field in place of the instance name field. Without this we get errors due to terraform thinking that the datasource is being modified due to the mapping of the instance_name and workspace_name fields

[lantoli]

this means that if user changes workspace_name, Delete and Create will be called instead of Update.

qq: is this the behavior we want? there are 3 main alternatives:

• Destroy and create a new resource (current approach)
• Handle workspace_name changes in Update
• Raise an error that workspace_name can't be updated

[kpatel71716]

Double checked and discussed with our team. This is intentional behavior. The same happens with the equivalent instance_name attribute today

[lantoli]

tf-validate PR check fail will probably get fixed if you bring latest from master to this branch, there was a fix here: #3713

[carriecwk]

lgtm

[LijieZhang1998]

LGTM 👍 Just have a few small things.

[kpatel71716]

Hi @lantoli @marcosuma . Apologies for the noise + delay on this PR, we found a bug after initial review that caused cascading issues trying to fix, but the PR is passing all checks now.

Would y'all be able to re-review this PR? Especially this change to preserver backwards compatibility with instance_name

[lantoli]

why do we have ID computed attribute? It was required in SDKv2 but it's not needed in TPF, for instance you can see the package paths:

"github.com/hashicorp/terraform-plugin-framework/types/basetypes"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/id"

ID makes sense if it's calculated and returned by the server - Atlas, but not so useful if generated in client, Terraform, and we're in TPF as this resource.

[kpatel71716]

Thanks for catching this. It seems I added unnecessary validation that led to errors that made me think I had to make changes here. I'll revert those to keep things much more simple

[lantoli]

can you remove ID attribute entirely?

[kpatel71716]

I am a bit confused on how that would work. I am probably missing something but from my understanding, we need to be able to set the instance_name/workspace_name fields or else we will always fail reading here

Is there a way around this using the basetypes and id packages above if we need the workspace/instance for the api call in the linked code?

[lantoli]

yes, my point is why we have id in the schema

[kpatel71716]

We need it avoid errors converting the schema to streamconnection.TFStreamConnectionModel. While the model needs the ID field so that the ImportStateRequest can have the information it needs in the ID field.

If we had access to the connection_name and instance_name in the ImportState() function then we wouldn't need to construct and parse an ID with that information. I am not sure if there is an example of this anywhere in the codebase though. fwiw the original implementation also contained ID so it's existed before these changes. But happy to create a follow-up ticket for any refactor once I understand if there is any way we can avoid using ID

[kpatel71716]

@lantoli let me know if you have any thoughts on the above. Happy to explore any ideas in a follow-up. Or we can circle back to it later

[lantoli]

I know this ID is not from this PR so feel free not to do anything in this PR.

I think there is some confusion, ID in the Import is not the schema attribute but the value passed to the import command or block, e.g:

terraform import mongodbatlas_stream_connection.test "DefaultInstance-12251446ae5f3f6ec7968b13-NewConnection"      # ID will be "DefaultInstance-12251446ae5f3f6ec7968b13-NewConnection" 

terraform import mongodbatlas_stream_connection.test "hello"      # ID will be "hello" 


import {
  to = mongodbatlas_stream_connection.test
  id = "my-id" # ID will be "my-id"
}

So this ID is passed to ImportState, current code is getting this value, splitting by dashes and populating the corresponding values, but there is no need for ID schema attribute in the resource:

func (r *streamConnectionRS) ImportState(ctx context.Context, req resource.ImportStateRequest, resp *resource.ImportStateResponse) {
	workspaceName, projectID, connectionName, err := splitStreamConnectionImportID(req.ID)
	if err != nil {
		resp.Diagnostics.AddError("error splitting stream connection import ID", err.Error())
		return
	}
	resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("instance_name"), workspaceName)...)
	resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("workspace_name"), workspaceName)...)
	resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("project_id"), projectID)...)
	resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("connection_name"), connectionName)...)
}

this implementation is correct, we get the "id" from import and fill it the main attributes, no need to fill it evertything as Read is called just after Import, so you only need to fill in the fields required by Read, as done in the current code.

but there is no need for ID in the resource schema or datasource schemas

[kpatel71716]

Ah ok, thank for you bearing with me and explaining! That makes sense to me, I'll create a follow-up ticket for our team to tackle this since to clean up our logic and link

[lantoli]

not probably for this PR, but we're preferring not to expose pagination parameter and instead just return the whole list of items

[kpatel71716]

that sgtm, I just took the existing pattern but will make a JIRA ticket for us to track cleaning this up