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: Fabric stream subscription #864

Merged
merged 19 commits into from
Feb 28, 2025
Merged

feat: Fabric stream subscription #864

merged 19 commits into from
Feb 28, 2025

Conversation

thogarty
Copy link
Contributor

  • Add stream subscription resource
  • Add data source by ids and data source all for subscriptions
  • Add docs and examples
  • Add acceptance tests for contribution

Copy link

codecov bot commented Feb 20, 2025

Codecov Report

Attention: Patch coverage is 57.02782% with 587 lines in your changes missing coverage. Please review.

Project coverage is 64.95%. Comparing base (6a231ed) to head (afc5e3b).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...l/resources/fabric/stream_subscription/resource.go 24.66% 267 Missing and 11 partials ⚠️
...nal/resources/fabric/stream_subscription/models.go 0.00% 234 Missing ⚠️
...urces/fabric/stream_subscription/datasource_all.go 24.39% 31 Missing ⚠️
...es/fabric/stream_subscription/datasource_by_ids.go 32.25% 21 Missing ⚠️
internal/resources/fabric/stream/sweeper.go 0.00% 10 Missing ⚠️
internal/fabric/testing_helpers/env_data.go 56.25% 4 Missing and 3 partials ⚠️
internal/framework/converters.go 0.00% 6 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #864       +/-   ##
===========================================
+ Coverage   31.38%   64.95%   +33.57%     
===========================================
  Files         217      224        +7     
  Lines       27667    29023     +1356     
===========================================
+ Hits         8682    18851    +10169     
+ Misses      18838     9275     -9563     
- Partials      147      897      +750     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines 19 to 20
type DataSourceByIDsModel struct {
ID types.String `tfsdk:"id"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit / FYI: there is no need to export model types; if you start them with a lower-case letter they will only be accessible within the resource package, which reduces the risk of us incorrectly or unexpectedly tying different resource packages together.

}

// Use API client to get the current state of the resource
streamSubscriptions, _, err := client.StreamSubscriptionsApi.GetStreamSubscriptions(ctx, data.StreamID.ValueString()).Limit(limit).Offset(offset).Execute()
Copy link
Contributor

Choose a reason for hiding this comment

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

Offset and Limit are optional parameter for API request. Need to update the logic where, we will pass these attributes only when value is NotNull

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are optional, but they also default to 0 and 20. Which are the defaults of the API anyway. So whether or not they are given the API result is always correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ValueInt32 will return 0 for a null HCL value on that same model attribute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because of Go Zero values: https://go.dev/tour/basics/12

Description: "Details of the last change on the stream resource",
Computed: true,
CustomType: fwtypes.NewObjectTypeOf[ChangeLogModel](ctx),
PlanModifiers: []planmodifier.Object{
Copy link
Contributor

@srushti-patl srushti-patl Feb 26, 2025

Choose a reason for hiding this comment

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

Can you remove PlanModifiers for change_log attribute as it's values are going to change? Also, remove it from other attributes where it's not required.

srushti-patl
srushti-patl previously approved these changes Feb 26, 2025
Copy link
Contributor

@srushti-patl srushti-patl left a comment

Choose a reason for hiding this comment

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

Approving PR but need to commit doc changes.
Getting this error: Uncommitted changes detected. Run 'make docs' and commit changes

@thogarty
Copy link
Contributor Author

@ctreatma , this is ready for another look from devrel group 👍

@srushti-patl srushti-patl self-requested a review February 26, 2025 22:02
srushti-patl
srushti-patl previously approved these changes Feb 26, 2025
@thogarty
Copy link
Contributor Author

@ctreatma , any issues with this PR?

@ctreatma
Copy link
Contributor

I think the only thing in here that forced devrel review was the new converter; that looks fine to me. (The rest looks fine too but will have to be reviewed by Fabric as codeowners.)

@thogarty
Copy link
Contributor Author

I think the only thing in here that forced devrel review was the new converter; that looks fine to me. (The rest looks fine too but will have to be reviewed by Fabric as codeowners.)

Yes, correct. Srushti has reviewed from Fabric but we just needed the last stamp. Thank you!

@srushti-patl srushti-patl self-requested a review February 28, 2025 18:36
@thogarty thogarty merged commit 15d399b into main Feb 28, 2025
12 of 14 checks passed
@thogarty thogarty deleted the fabric_stream_subscription branch February 28, 2025 18:38
Copy link

This PR is included in version 3.3.0 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/resources/fabric Issues related to Fabric and ECX APIs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants