-
Notifications
You must be signed in to change notification settings - Fork 38
Add initial implementation for common controller and tfstate parsing&conversion #2
Conversation
Signed-off-by: Hasan Turken <turkenh@gmail.com>
17c1f04
to
f0f6457
Compare
Signed-off-by: Hasan Turken <turkenh@gmail.com> (cherry picked from commit c416e6f9837010c29cc1506b93cee374c47e5bf9)
) | ||
|
||
const ( | ||
errUnexpectedObject = "The managed resource is not an Terraformed resource" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Error strings should not be capitalized.
https://github.com/golang/go/wiki/CodeReviewComments#error-strings
func (e *TerraformExternal) Create(ctx context.Context, mg xpresource.Managed) (managed.ExternalCreation, error) { | ||
u, err := e.Update(ctx, mg) | ||
return managed.ExternalCreation{ | ||
ExternalNameAssigned: meta.GetExternalName(mg) != "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this external name always be assigned by Terraform at first? The intention of this field is that it should only be true
if the external name was assigned somewhere inside of the Create
call.
if dr.Completed { | ||
return nil | ||
} | ||
return errors.Errorf("still deleting") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's going on here? Do we need to wait? 🤔
ar, err := e.executor.Apply(ctx, tr) | ||
if err != nil { | ||
return managed.ExternalUpdate{}, errors.Wrap(err, "failed to apply") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to wait for ar.Completed
?
) | ||
|
||
const ( | ||
AnnotationKeyStateMeta = "tf.crossplane.io/state-meta" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this file excluded from linting? It seems like it would fail due to missing commentary.
Thanks @negz for the review but this PR is intended to be a draft (just converted) to sync on a high-level structure. |
Closing in favor of #14 |
This PR adds an initial implementation (subject to change) for Terraform common controller interacting with the Scheduler interface.
It also adds support parse tfstate and primitives to do conversions.
I'll open a follow-up pr using this common controller in https://github.com/crossplane-contrib/provider-tf-aws repo.