fixing name for nextcloud deployment in collabora install script #285
Conversation
wejdross
changed the title
| @@ -540,7 +540,7 @@ func createInstallCollaboraJob(comp *vshnv1.VSHNNextcloud, svc *runtime.ServiceR | |||
| Command: []string{ | |||
| "bash", | |||
| "-cefx", | |||
| fmt.Sprintf("oc exec -i deployments/%s -- /install-collabora.sh \"%s\" \"%s\" \"%s\"", comp.GetName(), svc.Config.Data["isOpenshift"], comp.GetName(), comp.Spec.Parameters.Service.Collabora.FQDN), | |||
| fmt.Sprintf("oc exec -i deployments/%s -- /install-collabora.sh \"%s\" \"%s\" \"%s\"", comp.GetName()+"-nextcloud", svc.Config.Data["isOpenshift"], comp.GetName(), comp.Spec.Parameters.Service.Collabora.FQDN), | |||
There was a problem hiding this comment.
please update and use comp.GetWorkloadName()
There was a problem hiding this comment.
This funtion returns GetName()
func (v *VSHNNextcloud) GetWorkloadName() string {
return v.GetName()
}
why do we even keep such method?
There was a problem hiding this comment.
I just commited updated code to GetWorkloadName(), but I'm really unsure if that's the way we should go
There was a problem hiding this comment.
TLDR: we have one single place for getting the name of the workload so that we reduce potential bugs in the future.
During a feature request we needed the workload names of all our services. Later the implementation changed and we were not relying on on the workload names anymore. We still agreed to keep the functions anyway for the reason of this PR you opened. Unfortunately if you take a look at all our service api implementations you will realise that not all services have their workload name comp.GetName(). You have to see these methods as constants for each service and most of them luckily have the same name as comp.GetName(). To keep our code bug free, whenever there are constants we need to put them in our APIs.
Summary
Checklist
bug,enhancement,documentation,change,breaking,dependencyas they show up in the changelog