-
Notifications
You must be signed in to change notification settings - Fork 730
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
check error type instead of string #145
check error type instead of string #145
Conversation
Signed-off-by: Craig Tabita <ctab@google.com>
6ee54c2
to
a90bebc
Compare
@@ -130,8 +129,7 @@ func (r *ReconcileConstraint) Reconcile(request reconcile.Request) (reconcile.Re | |||
// Handle deletion | |||
if containsString(finalizerName, instance.GetFinalizers()) { | |||
if _, err := r.opa.RemoveConstraint(context.Background(), instance); err != nil { | |||
// TODO make the unrecognized constraint error detectable via class introspection | |||
if !(strings.HasPrefix(err.Error(), "Constraint kind ") && strings.HasSuffix(err.Error(), " is not recognized")) { | |||
if reflect.TypeOf(err).String() == "client.UnrecognizedConstraintError" { |
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.
Type switching would be the better approach (no strings):
if _, ok := err.(client.UnrecognizedConstraintError); ok {
}
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.
Thanks @maxsmythe - just changed it
Signed-off-by: Craig Tabita <ctab@google.com>
@@ -130,8 +128,7 @@ func (r *ReconcileConstraint) Reconcile(request reconcile.Request) (reconcile.Re | |||
// Handle deletion | |||
if containsString(finalizerName, instance.GetFinalizers()) { | |||
if _, err := r.opa.RemoveConstraint(context.Background(), instance); err != nil { | |||
// TODO make the unrecognized constraint error detectable via class introspection | |||
if !(strings.HasPrefix(err.Error(), "Constraint kind ") && strings.HasSuffix(err.Error(), " is not recognized")) { | |||
if _, ok := err.(*opa.UnrecognizedConstraintError); ok { |
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.
Currently opa.UnrecognizedConstraintError is a value, not a pointer, so this type check would never be true.
We should actually update the frameworks library to use a pointer-to-a-string, much like:
Signed-off-by: Craig Tabita <ctab@google.com>
…b/gatekeeper into check_for_unrecognized_template_type Signed-off-by: Craig Tabita <ctab@google.com>
No description provided.