Added the tier support to boutiqueshop operator #1
Conversation
| @@ -25,7 +25,9 @@ type BoutiqueShopSpec struct { | |||
| // LoadGeneratorUsers specifies how many fake users the load generator | |||
| // should simulate. When nil, the load generator Deployment will not run. | |||
| // +kubebuilder:validation:Minimum=0 | |||
| LoadGeneratorUsers *int `json:"loadGeneratorUsers,omitempty"` | |||
| LoadGeneratorUsers *int `json:"loadGeneratorUsers,omitempty"` | |||
| Tier string `json:"tier"` | |||
There was a problem hiding this comment.
We should have doc strings for each of these fields. We can also add a validation string to limit the choices, like the one you see above LoadGeneratorUsers.
| @@ -0,0 +1,23 @@ | |||
| apiVersion: v1 | |||
| kind: Namespace | |||
There was a problem hiding this comment.
Why introduce a hard-coded namespace here? Typically when doing development and demos, it's easiest to have an active namespace and just use it.
The concept of creating a tenant-specific namespace is out of scope for this operator, but in-scope for the automation that uses this operator to deploy a shop on behalf of an entry in the tenant management system.
| @@ -102,6 +96,12 @@ func (r *BoutiqueShopReconciler) components() []component { | |||
| {"RedisService", r.newRedisService}, | |||
| {"ShippingDeployment", r.newShippingDeployment}, | |||
| {"ShippingService", r.newShippingService}, | |||
| {"FrontendDeployment", r.newFrontendDeployment}, | |||
There was a problem hiding this comment.
This was re-ordered just because of the bug below where the loop stopped early, right? I think it's better to keep these sorted.
| if _, ok := resource.object.(*routev1.Route); ok { | ||
| if !r.RouteAvailable { | ||
| return ctrl.Result{}, nil | ||
| //return ctrl.Result{}, nil |
There was a problem hiding this comment.
It's ok to just delete this line.
| @@ -163,8 +163,8 @@ func (r *BoutiqueShopReconciler) statusURL(ctx context.Context, instance *demov1 | |||
|
|
|||
| route := routev1.Route{} | |||
| nn := types.NamespacedName{ | |||
| Name: routeName(instance), | |||
| Namespace: instance.Namespace, | |||
| Name: instance.Spec.TenantPrefix, | |||
| SeccompProfile: &corev1.SeccompProfile{ | ||
| Type: corev1.SeccompProfileTypeRuntimeDefault, | ||
| }, | ||
| // AllowPrivilegeEscalation: pointer.Bool(false), |
There was a problem hiding this comment.
Same here: what's the plan to re-enable this?
| func adName(instance *demov1alpha1.BoutiqueShop) string { | ||
| return instance.Name + "-ad" | ||
| func adName() string { | ||
| return "adservice" |
There was a problem hiding this comment.
Why hard-code all the names? Including the instance name enables the possibility that more than one BoutiqueShop resource can exist in the same namespace. That's not one of our central use cases, but it is a normal k8s use case, and I'm not aware of a reason for us to make that unusable.
If we hard-coded the names of resources, we'd need more advanced validation and/or error handling to deal with the possibility of someone trying to create two BoutiqueShop resources in the same namespace.
| @@ -15,7 +15,7 @@ func (r *BoutiqueShopReconciler) newFrontendRoute(ctx context.Context, instance | |||
| route := &routev1.Route{ | |||
| TypeMeta: metav1.TypeMeta{APIVersion: "route.openshift.io/v1", Kind: "Route"}, | |||
| ObjectMeta: metav1.ObjectMeta{ | |||
| Name: routeName(instance), | |||
| Name: instance.Spec.TenantPrefix, | |||
There was a problem hiding this comment.
What's the intent of this change?
| return r.newMethod(ctx, instance) | ||
| } | ||
|
|
||
| func (r *BoutiqueShopReconciler) newMethod(ctx context.Context, instance *demov1alpha1.BoutiqueShop) (*appResource, error) { |
| } | ||
|
|
||
| func (r *BoutiqueShopReconciler) getNs(instance *demov1alpha1.BoutiqueShop, component string) string { | ||
| ns := tierToComponentNsDict[instance.Spec.Tier][component] |
There was a problem hiding this comment.
If I'm understanding correctly, there are four namespaces that hold shared services: boutique-ops, enterprise-utilities, boutique-free and boutique-silver. A single instance of a service may run in one of those namespaces and be shared by many tenants.
I thought we agreed that the free and silver tiers wouldn't work, because the application itself has no native multi-tenancy. Did something change to suggest it will work?
No description provided.