Conversation
|
/lgtm |
|
New changes are detected. LGTM label has been removed. |
| return spec | ||
| } | ||
| spec = r.configureHttps(spec) | ||
|
|
There was a problem hiding this comment.
nit: I'd move the route creation in another place since this code is specific for creating the KbsConfigSpec
There was a problem hiding this comment.
You have added the route code into the buildKbsConfigSpec function. This code is specific for building the KbsConfigSpec instance, but the route is not part of it. Move this code to the Reconcile function, just above this line?
r.trusteeConfig.Status.IsReady = true
| // createOrUpdateKbsRoute creates a route for the KBS service if it doesn't already exist | ||
| // If the route already exists, this function does nothing | ||
| func (r *TrusteeConfigReconciler) createOrUpdateKbsRoute(ctx context.Context, termination routev1.TLSTerminationType) error { | ||
| routeName := KbsRouteName |
There was a problem hiding this comment.
I'd follow the same name convention as other objects, otherwise we could have issues when having multiple TrusteeConfig in the same namespace.
For example r.trusteeConfig.Name + "-kbs-route"
There was a problem hiding this comment.
yes but the service is hardcoded anyways, so wouldn't be the same issue with the service?
There was a problem hiding this comment.
ok fair enough, we can address this in another PR
When Trustee is started, if there is no existing route, create a passthrough when restrictive mode and the https certs are given, otherwise create an edge one. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
lmilleri
left a comment
lmilleri
left a comment
There was a problem hiding this comment.
What happens with different service types, e.g. ClusterIp or NodePort. Does the route need to change?
| return spec | ||
| } | ||
| spec = r.configureHttps(spec) | ||
|
|
There was a problem hiding this comment.
You have added the route code into the buildKbsConfigSpec function. This code is specific for building the KbsConfigSpec instance, but the route is not part of it. Move this code to the Reconcile function, just above this line?
r.trusteeConfig.Status.IsReady = true
| // createOrUpdateKbsRoute creates a route for the KBS service if it doesn't already exist | ||
| // If the route already exists, this function does nothing | ||
| func (r *TrusteeConfigReconciler) createOrUpdateKbsRoute(ctx context.Context, termination routev1.TLSTerminationType) error { | ||
| routeName := KbsRouteName |
There was a problem hiding this comment.
ok fair enough, we can address this in another PR
3 participants
When Trustee is started, if there is no existing route, create a passthrough when restrictive mode and the https certs are given, otherwise create an edge one.