old_mixer_repo
old_mixer_repo copied to clipboard
istio
Few codegen issues
#1. This comment in a generated file:
// Fully qualified name of this template const TemplateName = "listentry"
should be updated from "this template" to "the template"
#2. I think we should introduce a Name field in the Type object which holds the name of the type. Note: would this cause problems if a template author chooses to name a field "Name"? Do we have any reserved field names for templates?
#3. I think the Name field in Instances probably should be called TypeName instead. At some point, we will likely support multiple instances for a given type, each instance would potentially carry a name at that point, independent of the type's name (so we'd want Name and TypeName). Anyway, I think TypeName has a stronger semantic meaning in today's world anyway. Same note as above regarding reserved field names in templates ("TypeName" and "Name", any others?)
For #1 : yes #2 : Name is reserved, so we can add Name to Type. Curious: What is the need for a Name in Type ? The map we pass to the Configure holds the Type name in its key. Is there a specific use case you are trying to enable that needs Type to have a name ? #3: 'TypeName' instead of 'Name' probably make sense.
@geeknoid just to confirm, the operator's world will still remain the same right. Instances
- name: foo templateName : ...
You are proposing we change Instance struct to have TypeName instead of Name, which would mean the Operator's Instance.name field will make to Instance.typeName Field that adapter will receive right ? And similar for Type, for the current world of one type per instance, the operator's Instance.Name will map into Type.Name of the Type object that adapters will receive. Right ?
For #2, adding the Type.Name field is for good style. It's just not great to have a named object and the only place where it's name is kept is in an unrelated map key somewhere.
For #3, I think I'd like things to be consistent overall. In the operator's world, instances have the 'name' and the 'template' field. I'd like the Instance struct that adapters receive to also have a Name and a Template field (instead of TemplateName, in order to be consistent)
@mandarjog FYI
Chatted with @ZackButcher on this.
- Adding Type.Name can be added later post 0.2
- Having shared Types is not supported right now, therefor adding Type.Name along with Instance.TypeName is not critical right now. Also, changing adapters to use Instance.TypeName instead of Instance.Name is a pretty big change that we cannot take now.
- Instance.TemplateName is also a backwards compatible change that we can add post 0.2. Also, what operators add in instance config is not TemplateName, but instead it is called Kind in the CRD definition.
Given the above reasoning, punting this issue completely and we can tackle it in 0.3
