Skip to content

TFECO-9166: Add support for ResourceIdentity#SchemaFunc to allow providers to save RAM #1459

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

Merged
merged 3 commits into from
Mar 27, 2025

Conversation

ansgarm
Copy link
Member

@ansgarm ansgarm commented Mar 27, 2025

No description provided.

@ansgarm ansgarm changed the title Add support for ResourceIdentity#SchemaFunc to allow providers to save RAM TFECO-9166: Add support for ResourceIdentity#SchemaFunc to allow providers to save RAM Mar 27, 2025
@ansgarm ansgarm marked this pull request as ready for review March 27, 2025 09:21
@ansgarm ansgarm requested a review from a team as a code owner March 27, 2025 09:21
@ansgarm ansgarm enabled auto-merge (squash) March 27, 2025 09:46
Copy link
Member

@austinvalle austinvalle left a comment

Choose a reason for hiding this comment

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

Had some comments about removing Schema, but that could certainly go in a follow-up PR if you prefer. Will approve since the code itself looks good 👍🏻

@@ -37,6 +37,12 @@ type ResourceIdentity struct {
// previous resource schemas.
Schema map[string]*Schema

// SchemaFunc is an optional function that returns the schema for the
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to just remove Schema now?

Also, since we'll likely put another alpha out, might make sense to add a changelog describing this breaking change between alphas

Copy link
Member

Choose a reason for hiding this comment

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

Also if we remove Schema we should update the comment here to reflect that it's the only way to define an identity schema 😄

Copy link
Member

Choose a reason for hiding this comment

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

Also if we remove Schema we should update the comment here to reflect that it's the only way to define an identity schema 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in #1461

@@ -70,3 +76,18 @@ type ResourceIdentity struct {
// identity schema version data for a managed resource instance. Values must
// align to the typing mentioned above.
type ResourceIdentityUpgradeFunc func(ctx context.Context, rawState map[string]interface{}, meta interface{}) (map[string]interface{}, error)

// SchemaMap returns the schema information for this Resource whether it is
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// SchemaMap returns the schema information for this Resource whether it is
// SchemaMap returns the schema information for this resource identity whether it is

Copy link
Member

Choose a reason for hiding this comment

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

Also same comment here if we remove the Schema field, to update the pkg comment

Copy link
Member Author

Choose a reason for hiding this comment

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

done in #1461

@ansgarm ansgarm merged commit dee0dd6 into main Mar 27, 2025
22 checks passed
@ansgarm ansgarm deleted the schema-identity-func branch March 27, 2025 14:33
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.

2 participants