-
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
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.
Added logical commits to refactor the docs, simplify var naming that was copy/pasted over + remove copied unused methods, and refactored the workspace/instance struct mapping logic for a better intermediate solution as we transition
| @@ -0,0 +1,175 @@ | |||
| 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.
Filenames should not have the resource name. New convention similar to streamprocessor instead (remove stream_workspace from the name)
| streamWorkspaceReq, diags := newStreamWorkspaceCreateReq(ctx, &plan) | ||
| if diags.HasError() { | ||
| resp.Diagnostics.Append(diags...) | ||
| return | ||
| } | ||
| apiResp, _, err := connV2.StreamsApi.CreateStreamWorkspace(ctx, projectID, streamWorkspaceReq).Execute() | ||
| if err != nil { | ||
| resp.Diagnostics.AddError("error creating resource", err.Error()) | ||
| return | ||
| } | ||
|
|
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 would try to re-use also the API calls, then we can just run instanceModel := streaminstance.Create(streamInstance, diags *diag.Diagnostics) and it returns the converted streaminstance.TFModel.
Then you can simply check diags.HasError() after call and only do the workspaceModel = FromInstanceModel(instanceModel)
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.
Would this imply that we need to convert the streamWorkspace object to a streamInstance to call this? I am thinking we could create a reusable method for newStreamWorkspaceCreateReq since we don't need to pass in the entire plan and can pass individual fields instead
I am fine keeping the stream instance and stream workspace separate at that level for now. As one day we might add features to stream workspace that we don't support for stream instance. So converting to streamInstance would cause us to lose the data from the fields only supported in 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.
Would this imply that we need to convert the streamWorkspace object to a streamInstance to call this?
Yes.
[... one day we might add features to stream workspace that we don't support for stream instance
This makes sense. Motivation for users to change.
Feel free to resolve
3247211 to
b833d7f
Compare
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.
Great job! Thank you for addressing the comments 👏
Description
CLOUDP-335390 As part of the effort to rename Atlas Stream Processing Instances to Workspaces, we are adding a new stream_workspace resource/datasource to replace stream_instance
Link to any related issue(s):
Type of change:
Required Checklist:
Further comments