Conversation
|
@infkf is this ready for review? |
henrysachs
left a comment
There was a problem hiding this comment.
H1: Update() should delete+create atomically
Update() only calls Delete() and relies on the next reconcile to re-create. This leaves the share removed for up to one poll interval. Existing controllers (ldapgrouplinks, protectedbranches) do delete+create within Update():
func (e *external) Update(ctx context.Context, mg resource.Managed) (managed.ExternalUpdate, error) {
if _, err := e.Delete(ctx, mg); err != nil {
return managed.ExternalUpdate{}, err
}
_, err := e.Create(ctx, mg)
return managed.ExternalUpdate{}, err
}H2: Nil pointer dereference in Create() and Delete()
ProjectID and GroupID are *string + +optional. Observe() guards against nil, but Create() and Delete() dereference them directly (*cr.Spec.ForProvider.GroupID). If reference resolution fails, this panics. Other controllers (members, badges) guard these — please add nil checks.
H3: Observe() doesn't handle 404 from GetProject
Response is discarded (_), so 404 can't be distinguished from auth/network errors. If the referenced project is deleted externally, this causes an infinite retry loop instead of gracefully reporting the resource as absent. Every other controller captures the response and checks clients.IsResponseNotFound(res):
p, res, err := e.client.GetProject(...)
if err != nil {
if clients.IsResponseNotFound(res) {
return managed.ExternalObservation{ResourceExists: false}, nil
}
return managed.ExternalObservation{}, errors.Wrap(err, errGetProject)
}|
@henrysachs Please take another look |
|
@infkf could you please sign the dco so i can also compare it in ci? |
cc2f92e to
f4308a6
Compare
done |
Signed-off-by: Ihor Novikov <ihor.novikov@assurance.com>
f4308a6 to
a7ff7b0
Compare
Signed-off-by: Ihor Novikov <ihor.novikov@assurance.com>
a7ff7b0 to
355b9ac
Compare
Description of your changes
Fixes #270
Adds
ProjectShareGroupinstance to work with projects and groupsI have:
make reviewable testto ensure this PR is ready for review.How has this code been tested