- 
                Notifications
    
You must be signed in to change notification settings  - Fork 208
 
feat: Add stream_workspace resource + ds to replace stream_instance #3832
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
base: master
Are you sure you want to change the base?
Conversation
2c62840    to
    19286c8      
    Compare
  
    | 
           Please review by individual commit to filter out all the docs updates. My thought was that we can take an initial approach to convert the stream workspace model to the stream instance model and back to reduce code duplication. Happy to take a different approach if the team does not feel comfortable with this approach As a follow-up to this PR we can consider refactoring this approach, but for now I felt it provides functionality without making the PR too complex. I mostly copied over a lot of the stream instance code to achieve the addition of the new resource and datasource  | 
    
| 
           APIx bot: a message has been sent to Docs Slack channel  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
| To migrate from `mongodbatlas_stream_instance` to `mongodbatlas_stream_workspace`, update your data source configuration: | ||
| 
               | 
          ||
| ```terraform | ||
| # Old (deprecated) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: have the recommended one first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kpatel71716 we usually have this as an upgrade guide doc, separated by the resource /  data-source doc (but we add a link).
For example: https://registry.terraform.io/providers/mongodb/mongodbatlas/latest/docs/guides/cluster-to-advanced-cluster-migration-guide (this is a bit more complex)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to add: we also provide an example in the examples/ repository folder to show exactly how to upgrade
| 
               | 
          ||
| You must set the following variables: | ||
| 
               | 
          ||
| - `atlas_client_id`: MongoDB Atlas Service Account Client ID | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add ending periods
| } | ||
| } | ||
| 
               | 
          ||
| type TFStreamsWorkspaceModel struct { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as it's already in package streamworkspace, we use TFModel better
| } | ||
| 
               | 
          ||
| // GetInstanceName returns the workspace name as instance name for API compatibility | ||
| func (m *TFStreamsWorkspaceModel) GetInstanceName() types.String { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find where GetInstanceName or SetInstanceName are used
| } | ||
| 
               | 
          ||
| func (r *streamsWorkspaceRS) Create(ctx context.Context, req resource.CreateRequest, resp *resource.CreateResponse) { | ||
| var streamsWorkspacePlan TFStreamsWorkspaceModel | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: as similar comments, you can just say plan as we're already in the stream workspace package and this is even a local func var
| config.RSCommon | ||
| } | ||
| 
               | 
          ||
| func (r *streamsWorkspaceRS) Schema(ctx context.Context, req resource.SchemaRequest, resp *resource.SchemaResponse) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as it's already in resource package we normally use (r *rs) for resources and (d *ds) for datasources
| ``` | ||
| 
               | 
          ||
| ```release-note:note | ||
| data-source/mongodbatlas_stream_instance: Deprecates the `mongodbatlas_stream_instance` datasource, use `mongodbatlas_stream_workspace` | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DeprecationMessage is missing in the deprecated resource/datasource schemas
| 
               | 
          ||
| # Resource: mongodbatlas_stream_instance | ||
| 
               | 
          ||
| ~> **DEPRECATED:** This resource is deprecated. Please use [`mongodbatlas_stream_workspace`](stream_workspace.md) instead. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ~> **DEPRECATED:** This resource is deprecated. Please use [`mongodbatlas_stream_workspace`](stream_workspace.md) instead. | |
| ~> **DEPRECATED:** This resource is deprecated. Please use [`mongodbatlas_stream_workspace`](stream_workspace) instead. | 
links in TF registry don't have .md
| To migrate from `mongodbatlas_stream_instance` to `mongodbatlas_stream_workspace`, update your data source configuration: | ||
| 
               | 
          ||
| ```terraform | ||
| # Old (deprecated) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kpatel71716 we usually have this as an upgrade guide doc, separated by the resource /  data-source doc (but we add a link).
For example: https://registry.terraform.io/providers/mongodb/mongodbatlas/latest/docs/guides/cluster-to-advanced-cluster-migration-guide (this is a bit more complex)
| ``` | ||
| 
               | 
          ||
| ```release-note:note | ||
| data-source/mongodbatlas_stream_instance: Deprecates the `mongodbatlas_stream_instance` datasource, use `mongodbatlas_stream_workspace` | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, we're also deprecating the plural
| 
               | 
          ||
| `mongodbatlas_stream_workspaces` describes the stream workspaces defined in a project. | ||
| 
               | 
          ||
| ~> **NOTE:** This data source is an alias for `mongodbatlas_stream_instances`. Use this data source for new configurations. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do we mean with alias? It might be confusing since we're saying mongodbatlas_stream_instances is deprecated
| 
               | 
          ||
| ## Migration from stream_instances | ||
| 
               | 
          ||
| To migrate from `mongodbatlas_stream_instances` data source to `mongodbatlas_stream_workspaces`, use the following `moved` block: | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment. Make a single upgrade guide and move all this content there.
| To migrate from `mongodbatlas_stream_instance` to `mongodbatlas_stream_workspace`, update your data source configuration: | ||
| 
               | 
          ||
| ```terraform | ||
| # Old (deprecated) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to add: we also provide an example in the examples/ repository folder to show exactly how to upgrade
| 
               | 
          ||
| To learn more, see the [Stream Workspace Documentation](https://www.mongodb.com/docs/atlas/atlas-sp/manage-processing-instance/#configure-a-stream-processing-instance). | ||
| 
               | 
          ||
| ## Migration from stream_instance | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sweet!
| @@ -0,0 +1,200 @@ | |||
| package streamworkspace | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just double checking: is this a copy paste of the streaminstance file?
| return | ||
| } | ||
| 
               | 
          ||
| func (r *streamsWorkspaceRS) ValidateConfig(ctx context.Context, req resource.ValidateConfigRequest, resp *resource.ValidateConfigResponse) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see usage of this or why it would be needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar to my comment, please use in general unexported/lowercase functions so unused functions will be flagged
| "github.com/mongodb/terraform-provider-mongodbatlas/internal/service/streaminstance" | ||
| ) | ||
| 
               | 
          ||
| var _ resource.ResourceWithConfigure = &streamsWorkspaceRS{} | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to avoid duplicating all the logic in these two resources.
I would instead abstract out the CRUD operations just after the model conversion so all the API calls and diagnostic handling is the same.
Then you can keep the streaminstance.TFModel --> steamworkspace.TFModel as a final step.
Description
Please include a summary of the fix/feature/change, including any relevant motivation and context.
Link to any related issue(s):
Type of change:
Required Checklist:
Further comments