Add ability to add features to existing feature groups in Sagemaker

[aShevc]

Description

This leverages newly added UpdateFeatureGroupRequest in order to introduce adding new features to feature groups.

NOTE: in order to enable feature additions, I have had to remove ForceNew attribute from feature_definition field in order to disable recreation of the feature group when the size changes, but replaced it with CustomizeDiff. However, I was not able to simultaneously force creating the new feature group in case if the existing feature groups change, it will only force feature groups recreation when the new size of feature definitions is smaller. The reason for not being able to detect change of existing feature definitions seems to be the improper handling of CustomizeDiff attribute for TypeList fields. I have added an issue into Terraform SDK Plugin repo in order to address this. However, while resolving this definitely might take some time, I have fallen back to the solution on returning an error in case if the existing feature groups change upon applying the plan.

At this time, as I have tried quite a bit of different approaches, as seen in the issue above, and I would largely appreciate suggestions on any alternatives, I will gladly try to apply worthwhile suggestions

Some of the potential alternatives on the plate include:

• Set feature definitions field as computed, try to utilize ComputedIf and/or SetNew in case if undesired feature groups change are introduced.
• Rework Feature group service as the one using Terraform Framework rather than Terraform SDK. This may be usefuls as PlanModifiers may potentially be helpful in resolving the issue.

However, from what I see from my experience with TypeList and CustomizeDiff function, it may happen that those changes won't bring the required effect. Specifically, redesigning a service to use Terraform Framework rather than SDK may be quite a big change, especially given relatively small impact.

Therefore, the second thing beyond discussing alternative solutions would be to determine the plausibility of a fallback option I introudced. Woud largely appreciate a feedback on this

TODO:

• Explore additional ways to force changes prior to attempting applying the update
• Acceptance tests

Relations

Closes #26809

References

Output from Acceptance Testing

$ make testacc TESTS=TestAccXXX PKG=ec2

...

[github-actions]

Community Note

Voting for Prioritization

• Please vote on this pull request by adding a 👍 reaction to the original post to help the community and maintainers prioritize this pull request.
• Please see our prioritization guide for information on how we prioritize.
• Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.

For Submitters

• Review the contribution guide relating to the type of change you are making to ensure all of the necessary steps have been taken.
• For new resources and data sources, use skaff to generate scaffolding with comments detailing common expectations.
• Whether or not the branch has been rebased will not impact prioritization, but doing so is always a welcome surprise.

[github-actions]

Welcome @aShevc 👋

It looks like this is your first Pull Request submission to the Terraform AWS Provider! If you haven’t already done so please make sure you have checked out our CONTRIBUTOR guide and FAQ to make sure your contribution is adhering to best practice and has all the necessary elements in place for a successful approval.

Also take a look at our FAQ which details how we prioritize Pull Requests for inclusion.

Thanks again, and welcome to the community! 😃

[aShevc]

Updated the pull request with the following approach:

Replaced initial CustomizeDiff function with the function showing errors in case if feature definitions of existing features change

Hence, currently, it will give an error in case if feature definitions change on plan. Still found no way for now to force changes at that stage.

Will proceed on working on acceptance tests and make it ready for review

[aShevc]

@YakDriver @DrFaust92 can I gently ask to have an initial look at this guys?

[DrFaust92]

aShevc looks good but we are still with the issue of needing to force change as it will make operator to target destroy/taint and it think its bad UX

[aShevc]

aShevc looks good but we are still with the issue of needing to force change as it will make operator to target destroy/taint and it think its bad UX

I understand, but it is really the only option we have at the moment until the issue in Terraform plugin SDK is fixed. Do you think we can move forward temporarily with this solution and adjust it later?

Also, if you have other thoughts on how we could better deal with this scenario, this would be largely appreciated. Thank you!

[DrFaust92]

If Maintainers are ok with this its up to them, as i personally opted to just not submit a similar PR im against half measures

[aShevc]

@ewbankkit would you mind re-approving the actions run? I have had the Lint formatting fixed, so it should build now. Thanks!

[aShevc]

@ewbankkit @DrFaust92 since all checks are authorized, would the team be ready to merge it in soon? Do you guys have a plan on releasing this? Please do let know. Thank you!

[mpwsh]

sorry to ping but any updates here? <3 would love to get this merged. FeatureGroup updates are a nightmare to handle through terraform as of now