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

fix: change streams to be a SetNestedAttribute #128

Merged
merged 2 commits into from
Jul 19, 2024

Conversation

perangel
Copy link
Contributor

@perangel perangel commented Jul 17, 2024

This commit modifies the connection resource's streams attribute to be a SetNestedAttribute instead of a List. This is so that we can properly compute plan's that treat re-ordering of elements (streams) as a noop (if no attributes of the stream have been changed).

Previously if you added a new stream to a connection resource in the middle of the list, or changed the order, the provider would compute that as a change that had to be applied.

Additionally, we use a customer SetPlanModifier that may be parameterized with a key which is used to uniquely identify the elements in the set.

NOTE: the current implementation of the plan modifier uniqueByKey assumes that the elements of the set are Objects. Since this is the only use case for this modifier, this seems reasonable for now.

Testing

If you would like to verify these changes yourself, and do not have access to our SpeakEasy account, you can checkout this branch and run the provider locally in debug mode.

go run main.go -debug

NOTE: This will output instructions for exporting a TF_REATTACH_PROVIDERS env var that you must set when invoking terraform so that it uses the local instance of the provider.

This commit modifies the connection resource's `streams` attribute to be
a `SetNestedAttribute` instead of a List. This is so that we can
properly compute plan's that treat re-ordering of elements (streams) as
a noop (if no attributes of the stream have been changed).

Previously if you added a new stream to a connection resource in the
middle of the list, or changed the order, the provider would compute
that as a change that had to be applied.

Additionally, we use a customer SetPlanModifier that may be
parameterized with a `key` which is used to uniquely identify the
elements in the set.

NOTE: the current implementation of the plan modifier `uniqueByKey`
assumes that the elements of the set are Objects. Since this is the only
use case for this modifier, this seems reasonable for now.
@@ -0,0 +1,2 @@
internal/provider/connection_resource.go
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since these files have been manually modified, we need to exclude them from generation.

NestedObject: schema.NestedAttributeObject{
Attributes: map[string]schema.Attribute{
"cursor_field": schema.ListAttribute{
Computed: true,
PlanModifiers: []planmodifier.List{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not entirely certain that we need to remove these plan modifiers, so I defer to someone with more background to correct me here if this is not the right approach

@szemek
Copy link
Contributor

szemek commented Jul 17, 2024

@perangel I compiled your changes and provider works fine 👌🏻
Out of curiosity, what edge cases your changes cover that are not included in my pull request #111 ?

@perangel
Copy link
Contributor Author

@perangel I compiled your changes and provider works fine 👌🏻 Out of curiosity, what edge cases your changes cover that are not included in my pull request #111 ?

There are some SpeakEasy SDK/Provider generation configurations that need to be in place so that when the code gets generated it doesn't clobber the changes (see: .genignore). I'm also not entirely certain that the custom plan modifier is strictly necessary. I had been following some guidance from the speakeasy team and should hopefully get a review from them soon.

Copy link
Contributor

@ThomasRooney ThomasRooney left a comment

Choose a reason for hiding this comment

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

Tested and works well for me! I've run this through the set of tests with reordering streams that I tried when it was just a SetNestedAttribute and everything looks good; infrastructure gets to a stable state.

@ThomasRooney
Copy link
Contributor

Note: for context on why this was necessary over-and-above just converting streams to a SetNestedAttribute, we ran into behaviour where terraform would always pick up a change under streams even after converting it to a Set. When UseStateForUnknown (the diff suppression plan modifier) was applied to try and mitigate this, we ran into hashicorp/terraform-plugin-framework#709; data misalignment of the streams (plan+state couldn't be combined and got jumbled up) after the Read() method in the RefreshState lifecycle step.

References:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants