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.

Sign up for GitHub

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

Jump to bottom

Store back the CR only if it has changed #249

Merged
merged 1 commit into from
Mar 4, 2019
Merged

Store back the CR only if it has changed #249

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 9 additions & 10 deletions pkg/controller/jaeger/jaeger_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package jaeger
import (
"context"
"fmt"
"reflect"
"strings"
"time"

Expand Down Expand Up @@ -94,6 +95,7 @@ func (r *ReconcileJaeger) Reconcile(request reconcile.Request) (reconcile.Result
instance.APIVersion = fmt.Sprintf("%s/%s", v1alpha1.SchemeGroupVersion.Group, v1alpha1.SchemeGroupVersion.Version)
instance.Kind = "Jaeger"

originalInstance := *instance
str := r.runStrategyChooser(instance)

logFields := instance.Logger().WithField("execution", execution)
Expand All @@ -109,16 +111,13 @@ func (r *ReconcileJaeger) Reconcile(request reconcile.Request) (reconcile.Result
return reconcile.Result{}, err
}

logFields.Info("Configured Jaeger instance")
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to remove it? We could indicate the changes on line 119

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I wanted to remove it. As someone managing the cluster, I'm not sure this would be useful at all in the day-to-day. It's certainly useful to get a positive confirmation in the beginning, but considering that there will be one change every 5 seconds (due to the metrics on #248), this would cause too much noise in the logs.


// See https://github.com/jaegertracing/jaeger-operator/issues/231
// Uncomment when the issue above is fixed
//
// we store back the changed CR, so that what is stored reflects what is being used
// if err := r.client.Update(context.Background(), instance); err != nil {
// logFields.WithError(err).Error("failed to store back the current CustomResource")
// return reconcile.Result{}, err
// }
if !reflect.DeepEqual(originalInstance, *instance) {
// we store back the changed CR, so that what is stored reflects what is being used
if err := r.client.Update(context.Background(), instance); err != nil {
logFields.WithError(err).Error("failed to store back the current CustomResource")
return reconcile.Result{}, err
}
}

return reconcile.Result{}, nil
}
Expand Down