Fix URL table for ingresses #1317
Conversation
| shallow( | ||
| <AccessURLTable {...defaultProps} ingressRefs={[{} as ResourceRef]} getResource={mock} />, | ||
| ); | ||
| expect(mock).toHaveBeenCalled(); |
There was a problem hiding this comment.
Shouldn't this be .toHaveBeenCalledWith(...) to be able to justify the name of the test? (ie. to show that it fetches ingresses at mount time)
| expect(mock).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("fetches new ingresses when received", () => { |
There was a problem hiding this comment.
s/when received/when new ingress refs received/ (just to be specific.
| const mock = jest.fn(); | ||
| shallow(<AccessURLTable services={[]} ingresses={[]} fetchIngresses={mock} />); | ||
| const wrapper = shallow(<AccessURLTable {...defaultProps} getResource={mock} />); | ||
| wrapper.setProps({ ingressRefs: [{} as ResourceRef] }); | ||
| expect(mock).toHaveBeenCalled(); |
There was a problem hiding this comment.
Ditto here. I realise this line didn't change, but it was previously a mock of the function fetchIngresses so it was obvious. now it's a mock of the more generic getResource, so it'd be clearer to show that it is indeed getting an Ingress resource.
| private fetchIngresses() { | ||
| // Fetch all related Ingress resources. We don't need to fetch Services as | ||
| // they are expected to be watched by the ServiceTable. | ||
| this.props.ingressRefs.forEach(r => this.props.getResource(r)); |
There was a problem hiding this comment.
We don't need to worry about re-fetching ingresses we've previously fetched - I assume?
There was a problem hiding this comment.
not really, it will just update the info
Description of the change
The component
AccessURLTableis now being mounted before the references to the ingress objects are populated. This caused the AccessURLTable to be empty. To avoid that, we are now fetching also the ingresses when there are changes in the list.Another subtle change is that instead of relying on the implementation of
fetchIngresses, which was defined in the container, I have moved the implementation to the component so it's clear what it's doing.Applicable issues
Fixes #1307