fix(kafka update): broken preview#1311
Conversation
|
Added a few todos for when cli will work with |
pkg/cmd/kafka/update/update.go
Outdated
| // creates a summary of what values will be changed in this update | ||
| // returns a formatted string. Example: | ||
| // owner: foo_user ➡️ bar_user | ||
| // to do: Needs update once reauthentication_enabled is incorporated |
There was a problem hiding this comment.
How reauthentication_enabled affects this preview?
There was a problem hiding this comment.
The methods generating preview iterate through the entire update struct, the broken preview did display reauthentication_enabled too.
There was a problem hiding this comment.
Yes. That iteration thru all elements of struct is core issue IMHO
There was a problem hiding this comment.
We need to filter out the nil properties of the update object.
pkg/cmd/kafka/update/update.go
Outdated
| newT := new.Type() | ||
|
|
||
| for i := 0; i < new.NumField(); i++ { | ||
| for i := 0; i < 1; i++ { |
There was a problem hiding this comment.
I'm just curious what this does, how it fixes it?
There was a problem hiding this comment.
This is to hide the reauthentication_nabled from preview.
There was a problem hiding this comment.
Yes I understand why it was done, I just did not yet understand how. I've debugged locally to understand it now.
This is a "quick fix" but once reauth update is possible we won't be able to just revert this change. If the update does not include and change to reauthentication_enabled, reverting this will still include it in the update summary.
I see that the value of ReauthenticationEnabled is nil, so perhaps we can check for nil before including it in the update summary?
This also assumes that the owner field will always be the first property of the update object, we should not rely on this.
pkg/cmd/kafka/update/update.go
Outdated
| newT := new.Type() | ||
|
|
||
| for i := 0; i < new.NumField(); i++ { | ||
| for i := 0; i < 1; i++ { |
There was a problem hiding this comment.
[Question] Why this change?
There was a problem hiding this comment.
It looks like in is not stable/subject to break when api return new data
pkg/cmd/kafka/update/update.go
Outdated
| // to do: Needs update once reauthentication_enabled is incorporated | ||
| func getUpdateObjValue(v reflect.Value) reflect.Value { | ||
| if v.Kind() == reflect.Struct { | ||
| vstruct := v.Field(0) |
There was a problem hiding this comment.
A better waywould be to get the value field instead of index. Do we need to iterate through this struct as well?
There was a problem hiding this comment.
This should work:
| vstruct := v.Field(0) | |
| vstruct := v.FieldByName("value") |
This should suffice, no need to iterate.
Though I'm starting to think going forward with reflection on this is a bad idea, as we are making assumptions that are tightly coupled with the SDK, which could introduce breaking changes in the future by changing the name from "value" and since this is not strongly typed, we would not catch it..
For now, once you update with the suggestion above, we can merge as it is blocking any releases. Could you create a follow up issue to address this? We can simplify it by operating directly on the SDK model properties instead.
There was a problem hiding this comment.
Done!
Ready for review.
rkpattnaik780
requested review from
craicoverflow
and removed request for
craicoverflow
3 participants
Closes #1302, #1224
Verification Steps
rhoas kafka update --owner foo_userType of change
Checklist