Skip to content
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

add a couple of suggestions #5

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions internal/controller/rulegroup/rulegroup.go
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,9 @@ func (c *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex
// Return false when the external resource exists, but it not up to date
// with the desired managed resource state. This lets the managed
// resource reconciler know that it needs to call Update.

// Provider doesn't handle errors in isUpToDate, instead of this it returns `false`, which may follow to
// unexpected behavior and infinite update without chance to understand what is wrong
ResourceUpToDate: isUpToDate(cr, observedRuleGroup),

// Return any details that may be required to connect to the external
Expand All @@ -175,6 +178,8 @@ func (c *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex
}

func (c *external) Create(ctx context.Context, mg resource.Managed) (managed.ExternalCreation, error) {
// The code inside Create and Update funcs is identical on 99%, probably it would be nice to move the common part to the
// dedicated func and then reuse it.
cr, ok := mg.(*v1alpha1.RuleGroup)
if !ok {
return managed.ExternalCreation{}, errors.New(errNotRuleGroup)
Expand Down Expand Up @@ -279,8 +284,11 @@ func (c *external) Delete(ctx context.Context, mg resource.Managed) error {
return errors.Wrap(err, "")
}

// It would be nice to decompose this func and add `error` to return variables
//
//gocyclo:ignore
func isUpToDate(cr *v1alpha1.RuleGroup, observedRuleGroup *rwrulefmt.RuleGroup) bool {
// if understand correctly `cr` can't be nil, so we will decrease cyclomatic complexity if remove it
if cr == nil || observedRuleGroup == nil {
return false
}
Expand Down
Loading