Add a new struct to replace ArrayOrString struct

[ywluogg]

With implementation of TEP 075 and 076 going on, especially introducing json objects into params and results, it worths discussion about whether we want a new struct to replace ArrayOrString, as the implementation of json objects will happen in ArrayOrString, but the name doesn't really reflect what it actually represents. With this discussion, we are exploring two options here to reduce library breakages, and have a better structure.

• replace ArrayOrString with brand new struct
  • pros:
    • Have the potential to redesign the structure of the struct, so that we can cover some other flexibilities outside of TEP 075 and 076
  • cons:
    • having a tailing old struct that would be similar to the new struct
• rename ArrayOrString
  • pros:
    • don't need to think about a new structure, and the current structure is still flexible, with the scope of TEP 075 and 076
  • cons:
    • current structure could be not flexible for some of TEPs that's not 075 and 076
      @vdemeester @pritidesai I'm trying follow up on the discussion we have about this topic in WG. I think based on the above pros and cons comparisons, I'm still leaning toward changing the name of the struct, rather than introducing a new struct.

Pinging @chuangw6 @Yongxuanzhang as well.

[vdemeester]

@ywluogg I think, in the end, it's all an implementation detail. What we can do, if we want to "take" our time and split the work into multiple PRs, is to create the new struct as we see it fit, write tests for it. And then switch ArrayOrString with it, and finally remove ArrayOrString. This would be the best of both world, creating the new struct but with the intent and plan of replacing ArrayOrString.

[pritidesai]

We can have a list of PRs. Here is what I am thinking:

• Create new structs along with tests - ParamTypes and ResultTypes both can inline the same struct TypeSpec which has string, array, and map (please feel free to rename if you can find better names).

• Switch params and results with this new type and at the same time, dashboard and CLI can switch using this new type. CLI heavily relies on the existing type:

• Remove legacy ArrayOrString

[pritidesai]

I had a conversation with @vinamra28 about this plan. It looks great except the last PR - removing legacy ArrayOrString. Since CLI and other projects relies on it heavily, we will maintain the legacy struct until we have approval from the dependent projects i.e. the dependent projects have transitioned away from this legacy struct.

[chuangw6]

I had a conversation with @vinamra28 about this plan. It looks great except the last PR - removing legacy ArrayOrString. Since CLI and other projects relies on it heavily, we will maintain the legacy struct until we have approval from the dependent projects i.e. the dependent projects have transitioned away from this legacy struct.

Thanks @pritidesai for bringing up this. We should definitely take the dependent projects into consideration. So to help the dependent projects to transition away from this legacy struct, a new struct will be needed while the legacy one is maintained. Is that right?

[vinamra28]

I had a conversation with @vinamra28 about this plan. It looks great except the last PR - removing legacy ArrayOrString. Since CLI and other projects relies on it heavily, we will maintain the legacy struct until we have approval from the dependent projects i.e. the dependent projects have transitioned away from this legacy struct.

Thanks @pritidesai for bringing up this. We should definitely take the dependent projects into consideration. So to help the dependent projects to transition away from this legacy struct, a new struct will be needed while the legacy one is maintained. Is that right?

@chuangw6 yes the legacy one needs to be maintained for some time along with the new one. Also dependent projects(atleast CLI) can start their migration once this is out as CLI project vendors the pipelines project.

[chuangw6]

Thanks @vinamra28!

If a new struct is created that embeds the legacy ArrayOrString and includes object type, the type for ParamSpec's Default field and Param's value field will be changed to the new struct. Would this affect the dependent projects?

Changing these two types means many places that use default and value will be switched to the new struct. So I am also trying to understand what are the key scopes/areas we should maintain for the legacy struct ArrayOrString.

[vdemeester]

Chiming here a tad:

if the shape of the api doesn't change aka:

• if the way we marshal to yaml for the same type doesn't change)
• if the name of the fields do not change
  … then it's not a problem for the CLI.

It's similar to "duck typing", if it quacks it's a duck. The go struct and internal representation of our type can change without breaking the api, and thus without breaking older clients. My assumption is that we would have that new struct be compatible with ArrayOrString (at least in how the current behavior of ArrayOrString is). This would make a switch of struct transparent for today's behavior. When we add additional "feature" to that new struct, then old client will work with old behavior but not new one, which is to be expected.

Note: the cli could be defining it's own set of struct representing the API (it would be even more true if the cli was written in another language). From pipeline what we need to be careful about is the API shape, not necessarily our go code and struct.

[ywluogg]

My assumption is that we would have that new struct be compatible with ArrayOrString (at least in how the current behavior of ArrayOrString is).

Combining all are considered above, let's have a more defined new struct here to make sure we understand the compatibility of the old struct in the same way.
The new struct should have the following:

type StructuredValue struct { 
        // Old fields
	Type      StructuredValueType `json:"type"` // Represents the stored type of ArrayOrString.
	StringVal string    `json:"stringVal"`
	// +listType=atomic
	ArrayVal  []string          `json:"arrayVal"`
	ObjectVal map[string]string `json:"objectVal"`
        // New fields that only work for new feature
        NewField NewfieldType
}

@vdemeester @pritidesai

[ywluogg]

/reopen accidentally closed the issue...

[ywluogg]

Update on this issue: synced offline with @pritidesai @vdemeester that we agree that the changes wouldn't break any of the existing APIs but will introduce breakages in library. We will go with renaming the old struct, as we already added objectType in ArrayOrString@

[ywluogg]

/assign chuangw6

[lbernick]

Closed by #5304