Skip to content
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.

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

validation: fix error string #1176

Merged
merged 2 commits into from
Feb 3, 2025

Conversation

shajmakh
Copy link
Member

Instead of pointers display readable string.

found multiple pools matches for node group 0xc00467dca0 but expected one. Pools found [0xc00129aa08 0xc00129ac78]

To:

found multiple pools matches for node group &{&LabelSelector{MatchLabels:map[string]string{test: common,},MatchExpressions:[]LabelSelectorRequirement{},} <nil> <nil> map[]} but expected one. Pools found [test1 test2]

@openshift-ci openshift-ci bot requested review from swatisehgal and Tal-or January 29, 2025 11:48
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 29, 2025
@shajmakh
Copy link
Member Author

/cc @rshemtov13

@openshift-ci openshift-ci bot requested a review from rshemtov13 January 29, 2025 11:48
@rshemtov13
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 29, 2025
@Tal-or
Copy link
Collaborator

Tal-or commented Jan 29, 2025

/hold
We can implement the String() function for NodeGroups which I see you already began here:

func (ng *NodeGroup) ToString() string {

Once we'll change the function name to String() instead of ToString() it will be called automatically every time we'll try to print NodeGroup as a string

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 29, 2025
@ffromani
Copy link
Member

99% of the cases we add ToString() methods and not String() methods because we are not yet ready to commit to a string representation or we want to reserve the chance to change.
In general using the common interfaces like String() is the right thing to do with the caveat above.

@Tal-or
Copy link
Collaborator

Tal-or commented Jan 29, 2025

Then we shall at least call nodeGroup.ToString() instead of printing it using %v

@shajmakh
Copy link
Member Author

/hold
Thanks for the feedback folks, this sounds like a better alternative, I'll update the PR.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 3, 2025
Instead of pointers display readable string.

```
found multiple pools matches for node group 0xc00467dca0 but expected one. Pools found [0xc00129aa08 0xc00129ac78]
```

To:

```
found multiple pools matches for node group &{&LabelSelector{MatchLabels:map[string]string{test: common,},MatchExpressions:[]LabelSelectorRequirement{},} <nil> <nil> map[]} but expected one. Pools found [test1 test2]
```

Signed-off-by: Shereen Haj <[email protected]>
Copy link
Member

@ffromani ffromani left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

thanks, we can improve the string representation of the annotations map later

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 3, 2025
Copy link
Contributor

openshift-ci bot commented Feb 3, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ffromani, shajmakh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ffromani
Copy link
Member

ffromani commented Feb 3, 2025

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 3, 2025
For empty values of a struct print them out as "nil" string as well as
pointers that are nil like PoolName.

Signed-off-by: Shereen Haj <[email protected]>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 3, 2025
@@ -226,15 +226,25 @@ func init() {

func (ngc *NodeGroupConfig) ToString() string {
if ngc == nil {
return ""
return "nil"
Copy link
Member

Choose a reason for hiding this comment

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

let's see what's the string representation of nil

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I followed the k8s convention as in https://github.com/kubernetes/apimachinery/blob/v0.31.2/pkg/apis/meta/v1/generated.pb.go#L4387 (and other String() methods)
On another hand, I think we can use stringbuilder or similar to construct the string instead of sprintf for such long string presentation.

Copy link
Member

Choose a reason for hiding this comment

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

ok, this is better. No need to change

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason for this is that when ToString is called with empty values,some of these (like the v1.Labelselector presenting the MCP selector of the node group) is shown as "nil" while the internal ones lie ng.Config empty value is "" which doesn't look good and may confuse the readers (us mostly).

}
ngc.SetDefaults()
return fmt.Sprintf("PodsFingerprinting mode: %s InfoRefreshMode: %s InfoRefreshPeriod: %s InfoRefreshPause: %s Tolerations: %+v", *ngc.PodsFingerprinting, *ngc.InfoRefreshMode, *ngc.InfoRefreshPeriod, *ngc.InfoRefreshPause, ngc.Tolerations)
}

func (ng *NodeGroup) ToString() string {
if ng == nil {
return ""
return "nil"
Copy link
Member

Choose a reason for hiding this comment

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

ditto

}
return fmt.Sprintf("PoolName: %s MachineConfigPoolSelector: %s Config: %s", *ng.PoolName, ng.MachineConfigPoolSelector.String(), ng.Config.ToString())

pn := "nil"
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@ffromani
Copy link
Member

ffromani commented Feb 3, 2025

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 3, 2025
@openshift-merge-bot openshift-merge-bot bot merged commit 6fa926b into openshift-kni:main Feb 3, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants