-
Notifications
You must be signed in to change notification settings - Fork 14
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
NETOBSERV-1150 Console plugin columns & filters as config #417
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #417 +/- ##
==========================================
- Coverage 58.09% 57.57% -0.52%
==========================================
Files 169 170 +1
Lines 7985 8048 +63
Branches 990 975 -15
==========================================
- Hits 4639 4634 -5
- Misses 3067 3139 +72
+ Partials 279 275 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
8c786f2
to
9e1c8ae
Compare
enable: true | ||
portNames: | ||
"3100": loki | ||
columns: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For all the config below: do you remember if you changed some content or if it's only moving around these strings? (Just to help me reviewing, because I can't see the diff)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed a bit the fields:
- fieldName -> field
- isSelected -> default
and introducedcalculated
field for more complex value binding.
Appart from that it's the same as before. I'm keeping the yaml updated according to parallel PRs in both example and operator PR. See:
netobserv/network-observability-operator#477 (comment)
const previousObj = usePrevious(obj); | ||
const previous = usePrevious({ obj, state: initState.current }); | ||
|
||
React.useEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the same logic here is done in netflow-traffic component .. would it make sense to move that in the parent, and pass it down to its children? (unless it's not as simple as that)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The init state of the tab is not the same as the init state of netflow traffic here.
I've replicated the getConfig
part that could be shared in a common parent but it will need a bit of refactoring and to find a way to discriminate tabs from page.
Currently we are pointing different components using "$codeRef": "netflowParent.default"
/ "$codeRef": "netflowTab.default"
If you want we can try that in a followup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no no that's fine, I thought it was really the same thing
Destination, | ||
None | ||
} | ||
export type FilterCategory = 'source' | 'destination'; | ||
|
||
export type TargetedFilterId = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if a user configures a filter with an id that isn't in that list? (e.g. imagine there's a new field we forgot to add here) - Is it still going to work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing. The columns are still working fine since it's javascript behind the scene.
However, note that some behaviors are still hardcoded by column id as written in the description above.
if (d.id.includes('namespace')) { | ||
getOptions = cap10(getNamespaceOptions); | ||
validate = k8sNameValidation; | ||
} else if (d.id.includes('name')) { | ||
validate = k8sNameValidation; | ||
} else if (d.id.includes('kind')) { | ||
getOptions = cap10(getKindOptions); | ||
validate = rejectEmptyValue; | ||
encoder = kindFiltersEncoder(`${isSrc ? 'Src' : 'Dst'}K8S_Type`, `${isSrc ? 'Src' : 'Dst'}K8S_OwnerType`); | ||
} else if (d.id.includes('resource')) { | ||
getOptions = cap10(getResourceOptions); | ||
validate = k8sResourceValidation; | ||
checkCompletion = k8sResourceCompletion; | ||
encoder = k8sResourceFiltersEncoder( | ||
`${isSrc ? 'Src' : 'Dst'}K8S_Type`, | ||
`${isSrc ? 'Src' : 'Dst'}K8S_OwnerType`, | ||
`${isSrc ? 'Src' : 'Dst'}K8S_Namespace`, | ||
`${isSrc ? 'Src' : 'Dst'}K8S_Name`, | ||
`${isSrc ? 'Src' : 'Dst'}K8S_OwnerName` | ||
); | ||
} else if (d.id.includes('address')) { | ||
validate = addressValidation; | ||
} else if (d.id.includes('port')) { | ||
getOptions = cap10(getPortOptions); | ||
validate = portValidation; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if, rather than that, it would be better to name the bunch of "validators" defined above (ie. the validation functions) and refer to these names in the config ? That would be more in line with the desired config-driven approach ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, I admit that things like that make it slighlty more complicated:
encoder = k8sResourceFiltersEncoder(
`${isSrc ? 'Src' : 'Dst'}K8S_Type`,
`${isSrc ? 'Src' : 'Dst'}K8S_OwnerType`,
`${isSrc ? 'Src' : 'Dst'}K8S_Namespace`,
`${isSrc ? 'Src' : 'Dst'}K8S_Name`,
`${isSrc ? 'Src' : 'Dst'}K8S_OwnerName`
);
I don't know ... up to you :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. As same as columns, we need to find a tradeoff between config and hardcoded switches.
For now I choosed a simple approach for simplicity to introduce the config without breaking changes.
Ideally, I would like to have a way to write predefined javascript functions as same as calculated
field from columns with more capabilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
/lgtm |
New image: It will expire after two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=11f1f1b make set-plugin-image |
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: 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 |
Description
This PR allow sending definitions from config for:
value
andsort
functions according tofield
orcalculated value
options
,encoder
andvalidation
from id keywords.It also move server and loki configs to the loaded configmap for consistency.
/!\ FYI some behaviors are still hardcoded by column id.
I suggest to slowly migrate to something more flexible according to our needs.
Dependencies
netobserv/network-observability-operator#477
Checklist
If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.