Do not count typeConfigs if it is error.#6527
Conversation
|
/lgtm Apologies, I misunderstood your original concern. We really need to get better test coverage on istioctl. A lot of this logic should be moved into packages that are tested and out of the mains. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gyliu513, liamawhite The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
All current implementations of List() return empty []model.Config if they return an err. It does not hurt to append an empty list. If someone implemented a List() that returned >0 elements AND an error I believe istioctl SHOULD display it. |
|
Thanks @esnible , got the List() here https://github.com/istio/istio/blob/master/pilot/pkg/config/kube/crd/client.go#L469-L470 , but I still prefer that we should add the check even the List() return empty []model.Config if they return an err , this can make the code more readable. |
|
/retest |
1 similar comment
|
/retest |
esnible
left a comment
There was a problem hiding this comment.
Agreed that this change makes the code easier to read, although the behavior is unchanged.
|
/retest |
|
@gyliu513: The following tests failed, say
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
* Revert "Remove v2 transition commands since everything is now v2 (#6665)" This reverts commit 6339eb6. * Revert "Pilot param clusterRegistriesNamespace should default to pilot namespace (#6446)" This reverts commit b9294f7. * Revert "iptable just "return" by uid as the parameter u indicates (#6561)" This reverts commit 22a0b88. * Revert "Remove node agent service, residue from flexvolume driver. (#6651)" This reverts commit db3da82. * Revert "Continuously reapply galley CA bundle to prevent overwrite (#6599)" This reverts commit f9e8fd8. * Revert "Do not count typeConfigs if it is error. (#6527)" This reverts commit eb1de31. * Revert "Make racetest green - Fixed data races and flakiness (#6625)" This reverts commit 30b8ecb. * Revert "Improve push squashing (#6641)" This reverts commit 399cd2d.
* Revert "Remove v2 transition commands since everything is now v2 (istio#6665)" This reverts commit 6339eb6. * Revert "Pilot param clusterRegistriesNamespace should default to pilot namespace (istio#6446)" This reverts commit b9294f7. * Revert "iptable just "return" by uid as the parameter u indicates (istio#6561)" This reverts commit 22a0b88. * Revert "Remove node agent service, residue from flexvolume driver. (istio#6651)" This reverts commit db3da82. * Revert "Continuously reapply galley CA bundle to prevent overwrite (istio#6599)" This reverts commit f9e8fd8. * Revert "Do not count typeConfigs if it is error. (istio#6527)" This reverts commit eb1de31. * Revert "Make racetest green - Fixed data races and flakiness (istio#6625)" This reverts commit 30b8ecb. * Revert "Improve push squashing (istio#6641)" This reverts commit 399cd2d.
This is a follow up PR for #6424 (comment)
/cc @esnible @liamawhite