Allow prevent_destroy meta-arg to be overridden per resource #159
Conversation
- Fixes crossplane-contrib/provider-jet-azure#97 Signed-off-by: Alper Rifat Ulucinar <[email protected]>
There was a problem hiding this comment.
Please see crossplane-contrib/provider-jet-azure#97 (comment) .
There was a problem hiding this comment.
@ulucinar I understand the necessity of this change now. I added a long comment section in the review to make it more clear but while I was writing it, I had some second thoughts because while the use case we have doesn't, the concept itself allows something that is very much against Crossplane conventions (deleting without user consent given we don't have interactive confirmation like TF does before taking action), which could cause developers to use it with not as much caution.
What would you think about excluding these resources (PostgreSQLConfiguration, MySQLConfiguration and possibly MSSQLConfiguration) from Jet Azure and let people use the implementation in provider-azure? I'd like us to see more resources with that exception so that it's worth handling at the top level and take the risk.
| @@ -181,7 +181,7 @@ func (e *external) Observe(ctx context.Context, mg xpresource.Managed) (managed. | |||
|
|
|||
| return managed.ExternalObservation{ | |||
| ResourceExists: true, | |||
| ResourceUpToDate: plan.UpToDate, | |||
| ResourceUpToDate: plan.UpToDate && plan.Exists, | |||
| // AllowDestroy controls whether the `prevent_destroy: true` stanza | ||
| // is included in the Terraform configuration for the resource. | ||
| // For certain resources like Azure's PostgreSQLConfiguration, | ||
| // we would like to allow replacement. | ||
| AllowDestroy bool |
There was a problem hiding this comment.
| // AllowDestroy controls whether the `prevent_destroy: true` stanza | |
| // is included in the Terraform configuration for the resource. | |
| // For certain resources like Azure's PostgreSQLConfiguration, | |
| // we would like to allow replacement. | |
| AllowDestroy bool | |
| // AllowRecreation controls whether the changes that require resource | |
| // re-creation, i.e. immutable fields, should be allowed, which is against | |
| // Crossplane Resource Model. | |
| // | |
| // This should be enabled only in cases where there is no point in lifecycle | |
| // of the resource where the API returns NotFound. For example, the | |
| // configurations that Azure PostgreSQLConfiguration resource changes | |
| // have defaults, hence they always exist and when you create a new | |
| // PostgreSQLConfiguration, it's always an importing process of the default | |
| // so the operation would have to be a re-creation since all parameters are | |
| // immutable. Otherwise, controller imports instead of creation since the | |
| // resource exists and ends up in dead-lock state since re-creation is no | |
| // allowed. | |
| // | |
| // For other cases, enabling this is strictly against Crossplane Resource Model | |
| // where the deletion of the external resource is issued if and only if the custom | |
| // resource in Kubernetes API is deleted. Use this parameter with high caution. | |
| AllowRecreation bool |
|
@ulucinar Another alternative could be to copy the implementation from the native provider. All the structure and flows of Jet providers are just like native implementation, so it should work OK if you'd prefer to have those resources in Jet Azure. |
|
Should we close this one (for now) given the original issue resolved without needing this? |
Description of your changes
Fixes #158
For certain Terraform resources like Azure's PostgreSQL server configuration, we should not be including the
lifecycle.prevent_destroymeta-arg for updates. This PR proposes a backwards-compatible change that allows us to override this behavior. The default behavior stays the same, unless explicitly configured for a resource, we always include (apart from deletions) theprevent_destroy: truemeta-argument in the generated configuration.I have:
make reviewableto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR if necessary.How has this code been tested
Tested using the example manifest of PostgreSQLConfiguration resource of
provider-jet-azureon top of crossplane-contrib/provider-jet-azure#98.