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

Fixes #24634: API to find usage of a node property in Directives #5557

Draft
wants to merge 6 commits into
base: branches/rudder/8.0
Choose a base branch
from

Conversation

ElaadF
Copy link
Member

@ElaadF ElaadF commented Mar 29, 2024

https://issues.rudder.io/issues/24634

find_usage_property

Pagination

pagination

Preamble

This PR has been made as a side project on my free time. I don't have a deadline, and I don't know when it will be done. It's started as a small test to see if this was not too much painful to develop, but it ends up being a little bit more polish that I was expecting. Feel free to comment, add remarks and review the work done, I will do my best to address them ASAP on my free time.

Goal

This feature aim to add discoverability on node's properties. Nodes' properties are an essential part of Rudder workflow, it's seem important to see if a property used or not and if yes to be able to visit the object that is using this property. I'm often

Journal

16/04/2024 - 92ab292 3e85145

  • Beautify the search icon, to make it consistent with the edit buttons, only appear when the table line is hovered
  • Make the find usage API private
  • Next step verify that a user with no required read rights doesn't have access to this feature

06/04/2024 - 89348fd

  • Pagination is added in the UI (see the gif) it was a bit tedious, the hardest part was to make the table scrollable and contains inside the modal, I have some ugly CSS to force the max-height and the rows width but... it works, that's all I need
  • Next step is to beautify the search button to display it only on hover

05/04/2024

  • I have been able to set up API and service testing, the logics are a bit redundant, but the API is here to make sure that it is responding as expecting, prevent breaking the UI. In the service testing, we can see what the service take into account for detecting a property's usage.
  • Next step is probably adding pagination to the UI, because some user can have a lot of Technique and Directive, we have to be able to scale in terms of UI for the table since this is a tiny popup.
    I'm also concerned about the performance issues to parse every directive's parameter and technique's parameter on large instance, need more reflection on this subject, later.

04/04/2024 - fe6096f

  • The feature is working as expected, I have been able to add API tests, but I have some issue to make working my service tests, apparently test_find_property_usage_in_technique is not initialized, making some other tests (who rely on NodeConfig) fail, and I cannot figure it why for the moment.

Todos :

  • Add pagination 89348fd
  • Unit testing 83ae097
  • Beautify find usage button 3e85145
  • Make the API private 92ab292
  • Verify Read access requirements for URL redirection and find usage
  • Load testing
  • Refactoring toJson

@ElaadF ElaadF marked this pull request as draft March 29, 2024 22:18
@ElaadF
Copy link
Member Author

ElaadF commented Mar 29, 2024

PR updated with a new commit

3 similar comments
@ElaadF
Copy link
Member Author

ElaadF commented Mar 29, 2024

PR updated with a new commit

@ElaadF
Copy link
Member Author

ElaadF commented Mar 31, 2024

PR updated with a new commit

@ElaadF
Copy link
Member Author

ElaadF commented Mar 31, 2024

PR updated with a new commit

@ElaadF ElaadF force-pushed the ust_24634/api_to_find_usage_of_a_node_property_in_directives branch from 34fecec to 5294643 Compare March 31, 2024 22:11
@ElaadF
Copy link
Member Author

ElaadF commented Mar 31, 2024

Commit modified

@ElaadF ElaadF force-pushed the ust_24634/api_to_find_usage_of_a_node_property_in_directives branch from 5294643 to 3247f92 Compare March 31, 2024 22:14
@ElaadF
Copy link
Member Author

ElaadF commented Mar 31, 2024

PR rebased

1 similar comment
@ElaadF
Copy link
Member Author

ElaadF commented Mar 31, 2024

PR rebased

@ElaadF
Copy link
Member Author

ElaadF commented Apr 1, 2024

PR updated with a new commit

@ElaadF
Copy link
Member Author

ElaadF commented Apr 1, 2024

Commit modified

@ElaadF ElaadF force-pushed the ust_24634/api_to_find_usage_of_a_node_property_in_directives branch from e2825c6 to 994811e Compare April 1, 2024 01:53
@ElaadF
Copy link
Member Author

ElaadF commented Apr 1, 2024

Commit modified

@ElaadF ElaadF force-pushed the ust_24634/api_to_find_usage_of_a_node_property_in_directives branch from 994811e to 0bdaec7 Compare April 1, 2024 01:56
@ElaadF
Copy link
Member Author

ElaadF commented Apr 1, 2024

PR updated with a new commit

@ElaadF
Copy link
Member Author

ElaadF commented Apr 1, 2024

Commit modified

@ElaadF ElaadF force-pushed the ust_24634/api_to_find_usage_of_a_node_property_in_directives branch from db32c9c to 9aef71b Compare April 1, 2024 13:45
@ElaadF
Copy link
Member Author

ElaadF commented Apr 1, 2024

Commit modified

@ElaadF ElaadF force-pushed the ust_24634/api_to_find_usage_of_a_node_property_in_directives branch from 9aef71b to 1f73634 Compare April 1, 2024 14:16
@ElaadF
Copy link
Member Author

ElaadF commented Apr 1, 2024

Commit modified

@ElaadF ElaadF force-pushed the ust_24634/api_to_find_usage_of_a_node_property_in_directives branch 2 times, most recently from b332534 to c141189 Compare April 1, 2024 14:30
@ElaadF
Copy link
Member Author

ElaadF commented Apr 1, 2024

Commit modified

1 similar comment
@ElaadF
Copy link
Member Author

ElaadF commented Apr 1, 2024

Commit modified

@ElaadF ElaadF force-pushed the ust_24634/api_to_find_usage_of_a_node_property_in_directives branch from c141189 to edd1d5d Compare April 1, 2024 14:32
} yield {
techniques.filter { t =>
// I'm only checking in the value of method parameters here
val methodParamValues = t.calls.flatMap(getParam).filter(p => p.contains(s"$${node.properties[$propertyName]"))
Copy link
Member

Choose a reason for hiding this comment

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

I think we somehow need to use the parsed version (but I don't have any idea how/where it is available)

Copy link
Member Author

Choose a reason for hiding this comment

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

If I remember, we have indeed a parsed version, and it is basically the same process, thanks for the feedback, I will try to modify that

@fanf
Copy link
Member

fanf commented Apr 2, 2024

Cool, looks promising!

Could you add tests, if possible :

  • at least at api level with yaml request / query output)
  • if possible at service unit test level too, so that it's easier to have an anti regression specification

@ElaadF
Copy link
Member Author

ElaadF commented Apr 3, 2024

PR updated with a new commit

@ElaadF
Copy link
Member Author

ElaadF commented Apr 4, 2024

PR updated with a new commit

@ElaadF
Copy link
Member Author

ElaadF commented Apr 4, 2024

Commit modified

@ElaadF ElaadF force-pushed the ust_24634/api_to_find_usage_of_a_node_property_in_directives branch from f0f377e to 90bc8f4 Compare April 4, 2024 21:23
@ElaadF
Copy link
Member Author

ElaadF commented Apr 5, 2024

PR updated with a new commit

2 similar comments
@ElaadF
Copy link
Member Author

ElaadF commented Apr 5, 2024

PR updated with a new commit

@ElaadF
Copy link
Member Author

ElaadF commented Apr 6, 2024

PR updated with a new commit

@ElaadF
Copy link
Member Author

ElaadF commented Apr 6, 2024

PR rebased

@ElaadF ElaadF force-pushed the ust_24634/api_to_find_usage_of_a_node_property_in_directives branch from 9a91dc7 to 1787266 Compare April 6, 2024 01:49
@ElaadF
Copy link
Member Author

ElaadF commented Apr 6, 2024

PR rebased

@ElaadF ElaadF force-pushed the ust_24634/api_to_find_usage_of_a_node_property_in_directives branch from 1787266 to 83ae097 Compare April 6, 2024 01:54
@ElaadF ElaadF force-pushed the ust_24634/api_to_find_usage_of_a_node_property_in_directives branch from 1c009e1 to f17b205 Compare April 6, 2024 20:55
@ElaadF ElaadF force-pushed the ust_24634/api_to_find_usage_of_a_node_property_in_directives branch from f17b205 to 8928b2f Compare April 6, 2024 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants