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

feat: Indication that some of the connectors mandatory properties are… #659

Merged
merged 1 commit into from
Jan 19, 2024

Conversation

igarashitm
Copy link
Contributor

@igarashitm igarashitm commented Jan 17, 2024

… not yet configured

Fixes: #126

Using Ajv and component schema for the validation.

3 concerns:

  1. diagram is not so frequently re-rendered, can/should we re-render it when CanvasForm updates the model?
  2. Ajv think empty string is OK for the required parameter, are we OK with that? maybe OK for the 1st iteration?
  3. we'l need a custom validator for expression/dataformat/loadbalancer, especially for expression we need to handle dialects such as expression.simple.expression vs simple - currently they're just ignored

UPDATE

  1. Removed Ajv usage and added custom validator in model-validation.service.ts
  2. CustomNode adds node status listener to the VisualizationNode to be notified when node status is updated, allows to update the node status immediately the required parameter is filled in config form, without re-rendering whole graph
  3. Use NodeStatus.warning to indicate missing required parameter, and NodeStatus.default for steps that doesn't have any validation error. NodeStatus.default doesn't show any status icon.
2024-01-18.12-32-32.mp4

@lordrip
Copy link
Member

lordrip commented Jan 17, 2024

3 concerns:

1. diagram is not so frequently re-rendered, can/should we re-render it when CanvasForm updates the model?
2. Ajv think empty string is OK for the required parameter, are we OK with that? maybe OK for the 1st iteration?
3. we'l need a custom validator for expression/dataformat/loadbalancer, especially for expression we need to handle dialects such as `expression.simple.expression` vs `simple` - currently they're just ignored

Point 1

No, I don't think we should re-render the Canvas component when model properties are updated, this will happen on every keystroke, and best case scenario, if we debounce this operation, we'll see a flickering, IMHO, we should avoid this situation as much as possible.

Maybe we should try to see this problem from a different perspective, we would like to only re-render the nodes that change, so what about updating a timestamp whenever the model changes, this way we could identify and cause a new render for the node, and this might also update the description as well.

Point 2 & 3

I think we might need to drop ajv, because I think we need a custom validator, for instance, ajv might think that a value {{property}} for an enum is wrong, which is technically true but for our use case it's a legit use case). I'd say, let's start by writing a simple validator that checks whether the required properties have values other than an empty string, and we iterate over it, maybe in the future we could enhance it with the environment's context. Also, as you mentioned, we're gonna need custom validators for expression/dataformat/loadbalancer

@igarashitm
Copy link
Contributor Author

igarashitm commented Jan 17, 2024

Maybe we should try to see this problem from a different perspective, we would like to only re-render the nodes that change, so what about updating a timestamp whenever the model changes, this way we could identify and cause a new render for the node, and this might also update the description as well.

That sounds good, what is the timestamp specifically? Is it something in @patternfly/react-topology or in kaoto-next?

Point 2 & 3

I think we might need to drop ajv

yep maybe better to do so from beginning, will do.

@igarashitm igarashitm force-pushed the 126 branch 2 times, most recently from fb9cdb6 to f03624e Compare January 18, 2024 02:20
@igarashitm
Copy link
Contributor Author

Trying to capture CanvasForm change for CustomNode re-render without luck... now the node status is updated when the other node is selected, but not immediately after the model change

https://github.com/KaotoIO/kaoto-next/pull/659/files#diff-52db7ad35d4c02efb94b4ea159f85918acc17b6be2ef38b2390ec97371cfef30R57-R60

https://github.com/KaotoIO/kaoto-next/pull/659/files#diff-00f950f5f4336ffa9d13d52ee7ab9d6f7acb0dd656717ae3a66ce3fc650c3a87R28-R30

@igarashitm igarashitm force-pushed the 126 branch 4 times, most recently from 6523ba7 to 1a1f066 Compare January 18, 2024 17:34
@igarashitm igarashitm marked this pull request as ready for review January 18, 2024 17:34
@igarashitm
Copy link
Contributor Author

Done

2024-01-18.12-32-32.mp4

@igarashitm
Copy link
Contributor Author

igarashitm commented Jan 18, 2024

Some of the story gets the warning node status icon, possibly because the requried parameter is in URI, looking

Update

this is caused by the required parameters specified in the URI - #674

@lordrip
Copy link
Member

lordrip commented Jan 19, 2024

@igarashitm I'm not 100% sure, but could be possible that azz node shouldn't be marked as a Warning, if I'm not mistaken, both properties are specified?
image

@lordrip
Copy link
Member

lordrip commented Jan 19, 2024

@igarashitm I'm not 100% sure, but could be possible that azz node shouldn't be marked as a Warning, if I'm not mistaken, both properties are specified?

Alright, if I "touch" the field, then it gets updated, so probably means that the values used by the validation are before the parsing

Copy link
Member

@lordrip lordrip left a comment

Choose a reason for hiding this comment

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

There's still a pending issue: #674

@lordrip lordrip merged commit 2647614 into KaotoIO:main Jan 19, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Indication that some of the connectors mandatory properties are not yet configured
2 participants