view # of pods in topology #3746
Conversation
openshift-ci-robot
added
size/L
|
cc @openshift/team-devconsole-ux |
openshift-ci-robot
added
size/XL
|
@vikram-raj error fixed. Please review again. |
|
@sahil143 this needs to rebase |
openshift-ci-robot
added
the
needs-rebase
There was a problem hiding this comment.
nit: we can have similar in key-value i.e searchQuery: SearchQuery
frontend/public/actions/ui.ts
Outdated
openshift-ci-robot
removed
the
needs-rebase
There was a problem hiding this comment.
The TopologyFilterBar is located at '../topology2/filters/TopologyFilterBar'
There was a problem hiding this comment.
Getting this error in Console
There was a problem hiding this comment.
@vikram-raj I'm not able to reproduce this error. Can you provide more information regarding this?
There was a problem hiding this comment.
@sahil143 This error shows up when I import Operator backed service.
There was a problem hiding this comment.
I am able to see this error too. resp.status is a optional attribute so add checks to prevent cannot read property of null error.
There was a problem hiding this comment.
@karthikjeeyar @vikram-raj Added a check. Can you confirm that this error is resolved. e14a5a7
There was a problem hiding this comment.
modelFor will not work for crd based models (such knative revisions, event sources).
| modelFor(data.dc.kind), | |
| modelFor(referenceFor(data.dc)), |
There was a problem hiding this comment.
This was causing the error in the console.
vikram-raj
left a comment
vikram-raj
left a comment
There was a problem hiding this comment.
The topology filter is not persisting after the page refresh.
It is working fine. |
There was a problem hiding this comment.
Can you add a type here?
There was a problem hiding this comment.
Use this var(--pf-chart-global--FontSize--sm)
There was a problem hiding this comment.
Use this var(--pf-chart-global--FontSize--sm)
There was a problem hiding this comment.
Use RevisionModel.kind instead of Revision string.
There was a problem hiding this comment.
Technically we don't want to have shared with a dependency on knative-plugin.
We actually need a better approach or an extension to the pod ring.
| @@ -0,0 +1,3 @@ | |||
| .odc-pod-status__chart-label { | |||
There was a problem hiding this comment.
This should be
| .odc-pod-status__chart-label { | |
| .odc-pod-set__chart-label { |
sahil143
force-pushed
the
pod-count
branch
2 times, most recently
from
00866d4 to
229435d
Compare
|
/retest |
|
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
andrewballantyne
left a comment
andrewballantyne
left a comment
There was a problem hiding this comment.
This can be a followup PR but the styling for the filter bar has an extra line through it:
Please be sure to log a ticket for this @sahil143
I seriously question the use of Redux here. @sahil143 can you give me some feedback to why this is in redux and local storage? This feels to double down on the same front.
There was a problem hiding this comment.
This doesn't exist and causes a crashing exception on load of Topology.
There was a problem hiding this comment.
@sahil143 Christian is referring to the throw will likely break the thread and more than likely end up in a white screen.
Error modal wouldn't be the end of the world (helps the user report the error for instance)... but since this is an access-check perhaps we just log the error in the console. This is unlikely to happen, but if it does, the user shouldn't have to reach for browser refresh. Perhaps gracefully fail the access check to a false state and leave it at that. It'll be rare, but if it happens, under no circumstances should we crash the Console application.
There was a problem hiding this comment.
| }: RootState): any => topology.get('filters'); | |
| }: RootState): { [filterName: string]: any } => topology.get('filters'); |
Not sure if we have a pre-defined filter structure... but we can at least showcase this will be a map. I see them all being boolean, is that an intended design? If so, make the any be boolean as well.
There was a problem hiding this comment.
I suspect this change was for organization of what data comes from the store... but there is no need to export it. I'm on the fence on if it was worth breaking it out to its own type.
There was a problem hiding this comment.
Hmmm - technically this is the wrong type. showPodCount is a boolean, and icon is an optional string. !true && value will be false. If we were using PropTypes, this would cause an error. TypeScript doesn't know any better because it's not strongly typed.
| icon={!workloadData.showPodCount && workloadData.builderImage} | |
| icon={!workloadData.showPodCount ? workloadData.builderImage : undefined} |
I'd prefer we make a stronger effort to make our code predictable. If I (the component) expect a string or nothing, then let us provide that. Otherwise code within' the BaseNode needs to understand the type isn't a strict contract, and that's a violation of TypeScript principles.
The more I think about it - the more I am irritated TypeScript doesn't catch this. ! anything is a boolean. It should be able to tell optional string !== boolean. 😕
There was a problem hiding this comment.
Is there any reason we don't want to elaborate on this state structure?
There was a problem hiding this comment.
Can't do this inside a reducer; this is a side-effect.
The reducer is a pure function that takes the previous state and an action, and returns the next state.
Docs: https://redux.js.org/basics/reducers/#handling-actions
We need to use middleware to provide this kind of side-effecty operations.
There was a problem hiding this comment.
ticket to address this https://issues.redhat.com/browse/ODC-2664
There was a problem hiding this comment.
I think we have a standard to use _.merge - surprised this isn't breaking a lint.
There was a problem hiding this comment.
Also a side-effect... see my other comment.
There was a problem hiding this comment.
This mutates DEFAULT_TOPOLOGY_FILTERS and that is not desired. Any imports of this will have a mutated value. Please make sure you avoid using lodash methods that mutate an object that is not to be mutated (constants, props, state, etc).
Note: This method mutates object.
Docs: https://lodash.com/docs/4.17.15#merge
A fix, for instance, could be:
| return merge(DEFAULT_TOPOLOGY_FILTERS, JSON.parse(filters)); | |
| return merge({}, DEFAULT_TOPOLOGY_FILTERS, JSON.parse(filters)); |
|
/lgtm cancel This is actually broken right now. |
initial commit use select instead of dropdown from pf and add pod count to the nodes add input field for search fix unit tests and add new unit tests add checks for pods and rc use value on input field from searchQuery ODC-2343: store filters in locaStorage https://jira.coreos.com/browse/ODC-2343 change SearchFilter to searchQuery update imports from topology2 to topology add checks for resp for checkPodEditAccess add support fo crd to get K8sKind add type for impersonate and add scss file for PodSet use reducer for topologyFilters using plugin extension refactor pod-ring utility refactor filter dropdown and change styles for filter bar fix locaal storage bug fix failing tests and remove setTraffic from the filters list change css class name remove podset.scss remove not extra line make requested changes
| filters: getTopologyFilters(state), | ||
| }); | ||
|
|
||
| const dispatchToProps = (dispatch: Dispatch): DispatchProps => ({ |
There was a problem hiding this comment.
I forgot to mention this, but it's not a needed change so it doesn't matter.
Feel free to use the short-hand dispatchToProps layout, which is just an object. If all you need to do is pass-through values and nothing else, you can use the short-hand:
const dispatchToProps = {
onFilterChange: setTopologyFilters,
};
Not sure what this means to our typing, but React-Redux handles this nicely.
| expect(wrapper.exists()).toBeTruthy(); | ||
| }); | ||
|
|
||
| it('should have 5 filters in total', () => { |
There was a problem hiding this comment.
We shouldn't have a number in the test description... it's easy for this to get out of sync (see my previous review comment to see what I named it)
| export type TopologyFilters = { | ||
| display: DisplayFilters; | ||
| searchQuery: SearchQuery; | ||
| }; |
There was a problem hiding this comment.
Probably shouldn't create types after they're used... but I suspect TypeScript is handling this. You cannot do this with variables, our linter errors out on it.
|
/kind feature |
openshift-ci-robot
added
the
kind/feature
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewballantyne, christianvogt, sahil143, vikram-raj 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 |
openshift-ci-robot
added
the
approved
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
|
/retest Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
13 participants
ODC-Story: https://jira.coreos.com/browse/ODC-2342
ODC-Story: https://jira.coreos.com/browse/ODC-2343
ODC-2343: store all the filters in local storage to presist them across user sessions
ODC-2342:
[TODO]:
This Pr adds