Skip to content

fix(kafka update): broken preview #1311

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rkpattnaik780 merged 4 commits into from
Nov 12, 2021
Merged

fix(kafka update): broken preview #1311

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b18caea
fix(kafka update): broken preview
rkpattnaik780 Nov 11, 2021
336dd0d
fix: change update summary logic
rkpattnaik780 Nov 11, 2021
63ca6d0
fix: get field by name
rkpattnaik780 Nov 12, 2021
58c1860
docs: improve error message
rkpattnaik780 Nov 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions pkg/cmd/kafka/update/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"fmt"
"reflect"
"strconv"
"strings"

"github.com/AlecAivazis/survey/v2"
kafkamgmtclient "github.com/redhat-developer/app-services-sdk-go/kafkamgmt/apiv1/client"
Expand Down Expand Up @@ -281,20 +282,23 @@ func promptConfirmUpdate(opts *options) (bool, error) {
// 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
Copy link
Collaborator

@wtrocki wtrocki Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How reauthentication_enabled affects this preview?

Copy link
Contributor Author

@rkpattnaik780 rkpattnaik780 Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The methods generating preview iterate through the entire update struct, the broken preview did display reauthentication_enabled too.

Copy link
Collaborator

@wtrocki wtrocki Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. That iteration thru all elements of struct is core issue IMHO

Copy link
Contributor

@craicoverflow craicoverflow Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to filter out the nil properties of the update object.

func generateUpdateSummary(new reflect.Value, current reflect.Value) string {
var summary string

newT := new.Type()

for i := 0; i < new.NumField(); i++ {
for i := 0; i < 1; i++ {
Copy link
Collaborator

@wtrocki wtrocki Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Question] Why this change?

Copy link
Collaborator

@wtrocki wtrocki Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like in is not stable/subject to break when api return new data

Copy link
Contributor

@craicoverflow craicoverflow Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just curious what this does, how it fixes it?

Copy link
Contributor Author

@rkpattnaik780 rkpattnaik780 Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to hide the reauthentication_nabled from preview.

Copy link
Contributor

@craicoverflow craicoverflow Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

field := newT.Field(i)
fieldTag := field.Tag.Get("json")
fieldName := field.Name

oldVal := getElementValue(current.FieldByName(fieldName)).String()
newVal := getElementValue(new.FieldByName(fieldName)).String()
newVal := getUpdateObjValue(new.FieldByName(fieldName)).String()

summary += fmt.Sprintf("%v: %v %v %v\n", color.Bold(fieldTag), oldVal, icon.Emoji("\u27A1", "=>"), newVal)
fieldTagDisp := strings.Split(fieldTag, ",")[0]

summary += fmt.Sprintf("%v: %v %v %v\n", color.Bold(fieldTagDisp), oldVal, icon.Emoji("\u27A1", "=>"), newVal)
}

return summary
Expand All @@ -308,3 +312,13 @@ func getElementValue(v reflect.Value) reflect.Value {
}
return v
}

// get the true value from a reflect.Value for KafkaUpdateRequest struct
// 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)
Copy link
Contributor Author

@rkpattnaik780 rkpattnaik780 Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A better waywould be to get the value field instead of index. Do we need to iterate through this struct as well?

Copy link
Contributor

@craicoverflow craicoverflow Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should work:

Suggested change
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.

Copy link
Contributor Author

@rkpattnaik780 rkpattnaik780 Nov 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!
Ready for review.

return vstruct.Elem()
}
return v
}