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

Refactor controller logic #784

Merged
merged 12 commits into from
Jul 2, 2019
Merged

Conversation

lburgazzoli
Copy link
Contributor

Common actions such as deep-cloning the resource and update
them are now performed by the controller wich results in
clean handlers

@lburgazzoli lburgazzoli force-pushed the controllers branch 3 times, most recently from 2798b38 to 158cc48 Compare July 1, 2019 08:42
@lburgazzoli lburgazzoli force-pushed the controllers branch 7 times, most recently from b67c6c6 to 25e7640 Compare July 1, 2019 18:36
@lburgazzoli
Copy link
Contributor Author

@astefanutti ready for review


target, err = a.Handle(ctx, target)
if err != nil {
return reconcile.Result{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't that be reconcile.Result{}, err to avoid swallowing the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


target, err = a.Handle(ctx, target)
if err != nil {
return reconcile.Result{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't that be reconcile.Result{}, err to avoid swallowing the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


target, err = a.Handle(ctx, target)
if err != nil {
return reconcile.Result{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't that be reconcile.Result{}, err to avoid swallowing the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


target, err = a.Handle(ctx, target)
if err != nil {
return reconcile.Result{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't that be reconcile.Result{}, err to avoid swallowing the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}

// Requeue scheduling build so that it re-enters the build working queue
if instance.Status.Phase == v1alpha1.BuildPhaseScheduling ||
instance.Status.Phase == v1alpha1.BuildPhaseFailed {
if targetPhase == v1alpha1.BuildPhaseScheduling || targetPhase == v1alpha1.BuildPhaseFailed {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

targetPhase may be empty if the schedule action returns nil, or possibly, when no action can handle the event, so it may be better to replace it with target.Status.Phase.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -189,7 +189,7 @@ func (r *ReconcileBuild) Reconcile(request reconcile.Request) (reconcile.Result,
if err != nil {
return reconcile.Result{
Requeue: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, error are re-queued by default already.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -256,7 +256,7 @@ func (r *ReconcileIntegration) Reconcile(request reconcile.Request) (reconcile.R
if err != nil {
return reconcile.Result{
Requeue: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, error are re-queued by default already.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -166,7 +166,7 @@ func (r *ReconcileIntegrationKit) Reconcile(request reconcile.Request) (reconcil
if err != nil {
return reconcile.Result{
Requeue: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, error are re-queued by default already.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -151,7 +151,7 @@ func (r *ReconcileIntegrationPlatform) Reconcile(request reconcile.Request) (rec
if err != nil {
return reconcile.Result{
Requeue: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, error are re-queued by default already.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@astefanutti astefanutti self-requested a review July 2, 2019 12:21
@lburgazzoli lburgazzoli merged commit b74a86c into apache:master Jul 2, 2019
@lburgazzoli lburgazzoli deleted the controllers branch July 2, 2019 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants