supporting graph generation based on input dependency #36

RinkiyaKeDad · 2021-04-23T11:49:01Z

Fixes #24

Support for generating a graph based on a dependency user enters
Coloring the main module incase of the graph of all dependencies
Removed some commented code which was left in added pretty printing json, storing all chains, Chains type, module rename, go bump #28

Signed-off-by: RinkiyaKeDad <arshsharma461@gmail.com>

cmd/graph.go

alenkacz · 2021-04-26T11:24:21Z

cmd/graph.go

+				}
+				// main module can never be a neighbour
+				for _, neighbour := range depGraph[dep] {
+					if dep == mainModule {


potentially you could extract the nodes creation to func and reuse it here as well as in the other branch

I have switched to strings.Join() for the if block but in the else block, reusing the code won't be possible IMO.

This is because for generating the graph based on an input dependency our .dot file looks like (the if block):

"A" -> "B" -> "C" -> "D" "E" -> "F" -> "G" -> "H" . . .

but for generating the graph of the entire dependency tree it looks like(the else block):

"A" -> "B" "B" -> "C" "C" -> "D" "E" -> "F" "F" -> "G" "G" -> "H"

Now we could have the second graph also look like the first one but for that we would need access to all the chains like we have incase of the first graph. Getting these chains using the getAllChains function is both time and memory intensive so it'd be better to avoid it. Thoughts?

Hmm I see, I think I am lacking an understanding what's the difference between the two branches 🤔 Why is it different?

So we support making two types of graphs:

based on a dependency a user enters

graph of all dependencies

To create the graph of all dependencies the logic is simple. We iterate through each dependency and then iterate through its neighbors and have all the edges of the graph in the .dot file like this:

"A" -> "B" "B" -> "C" "C" -> "D" "E" -> "F" "F" -> "G" "G" -> "H"

But for the graph based on the dependency entered by the user, we need access to all dependency chains. This is because we need to check which chain has the dependency entered by the user. Once we know that we simply put those chains in the .dot file:

"A" -> "B" -> "C" -> "D" "E" -> "F" -> "G" -> "H"

1. Why can't we reuse the logic by abstracting it to a function?

Reusing the logic would mean having the graph of all dependencies in a format similar to the graph based on the dependency entered by the user.

And to create this graph in a similar format, we would need access to all dependency chains.

2. What's the problem in getting access to all dependency chains?

To get these chains we call the getAllChains() function. This function is very demanding in terms of memory and also takes a lot of time, so we should ideally try to avoid calling it. Since we can get the graph of all dependencies without calling this function I didn't use it.

Does this make more sense? 😅 Also, I'm currently refactoring the code in this file into separate functions and will push that tomorrow, which would help seeing the difference a bit more clearly.

alenkacz · 2021-04-26T11:25:25Z

cmd/graph.go

+		fileContents := "strict digraph {\ngraph [overlap=false];\n"
+
+		// graph to be generated is based around input dep
+		if dep != "" {


I think we should also add some tests, I see that this whole function does not have any...

maybe let's do a function that received depGraph as parameter, and then we can assert output of both branches of the if here. WDYT?

I'm currently refactoring the code in this file into separate functions

Yup was doing that only :)

Signed-off-by: RinkiyaKeDad <arshsharma461@gmail.com>

RinkiyaKeDad · 2021-04-27T09:49:37Z

@alenkacz I've abstracted the logic for the if and else blocks into separate functions. Hope this provided some more clarity. I've also added tests for all the functions being used in that file 🙂

alenkacz

Last two nits, and we're there :) thanks for your patience!

alenkacz · 2021-04-27T15:05:30Z

cmd/graph.go

+// get the contents of the .dot file for the graph
+// when the -d flag is set
+func getFileContentsForSingleDep(chains []Chain, dep string) string {
+	data := ""


nit: you could skip this and start with what's on line 85

alenkacz · 2021-04-27T15:07:09Z

cmd/graph.go

+func getFileContentsForAllDeps(deps []string, depGraph map[string][]string, mainModule string) string {
+	data := ""
+	// color the main module as yellow
+	data += fmt.Sprintf("MainNode [label=\"%s\", style=\"filled\" color=\"yellow\"]\n", mainModule)


this is shared between the function, maybe we could extract it to func with dependency of module name

Signed-off-by: RinkiyaKeDad <arshsharma461@gmail.com>

RinkiyaKeDad · 2021-04-27T15:51:18Z

Thanks for reviewing @alenkacz! Made the changes. Please drop a "lgtm" to merge this if they're okay 🙃

alenkacz

🚢

alenkacz · 2021-04-27T15:56:55Z

/lgtm

k8s-ci-robot · 2021-04-27T15:56:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alenkacz, RinkiyaKeDad

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:

~~OWNERS~~ [RinkiyaKeDad,alenkacz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

RinkiyaKeDad added 2 commits April 23, 2021 17:12

initial commit

74252eb

Signed-off-by: RinkiyaKeDad <arshsharma461@gmail.com>

removed commented code from utils

e9a02f6

Signed-off-by: RinkiyaKeDad <arshsharma461@gmail.com>

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 23, 2021

k8s-ci-robot requested review from dims and nikhita April 23, 2021 11:49

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 23, 2021

RinkiyaKeDad requested review from alenkacz and removed request for dims and nikhita April 23, 2021 11:49

RinkiyaKeDad changed the title ~~24 graph for arg dep~~ supporting graph generation based on input dependency Apr 23, 2021

broken down the get chains function into three

ddb00a5

Signed-off-by: RinkiyaKeDad <arshsharma461@gmail.com>

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 24, 2021

alenkacz reviewed Apr 26, 2021

View reviewed changes

RinkiyaKeDad added 2 commits April 26, 2021 17:37

used strings.join for input dep graph

fcef694

Signed-off-by: RinkiyaKeDad <arshsharma461@gmail.com>

abstracted to functions, added tests

f5fcc73

Signed-off-by: RinkiyaKeDad <arshsharma461@gmail.com>

RinkiyaKeDad requested a review from alenkacz April 27, 2021 09:49

alenkacz reviewed Apr 27, 2021

View reviewed changes

abstracted coloring node to a function

94720db

Signed-off-by: RinkiyaKeDad <arshsharma461@gmail.com>

RinkiyaKeDad requested a review from alenkacz April 27, 2021 15:51

alenkacz approved these changes Apr 27, 2021

View reviewed changes

k8s-ci-robot assigned alenkacz Apr 27, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 27, 2021

k8s-ci-robot merged commit cd4c942 into kubernetes-sigs:main Apr 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

supporting graph generation based on input dependency #36

supporting graph generation based on input dependency #36

RinkiyaKeDad commented Apr 23, 2021

alenkacz Apr 26, 2021

RinkiyaKeDad Apr 26, 2021

alenkacz Apr 26, 2021

RinkiyaKeDad Apr 26, 2021

alenkacz Apr 26, 2021

alenkacz Apr 26, 2021

RinkiyaKeDad Apr 26, 2021

RinkiyaKeDad commented Apr 27, 2021

alenkacz left a comment

alenkacz Apr 27, 2021

alenkacz Apr 27, 2021

RinkiyaKeDad commented Apr 27, 2021

alenkacz left a comment

alenkacz commented Apr 27, 2021

k8s-ci-robot commented Apr 27, 2021

supporting graph generation based on input dependency #36

supporting graph generation based on input dependency #36

Conversation

RinkiyaKeDad commented Apr 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RinkiyaKeDad commented Apr 27, 2021

alenkacz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RinkiyaKeDad commented Apr 27, 2021

alenkacz left a comment

Choose a reason for hiding this comment

alenkacz commented Apr 27, 2021

k8s-ci-robot commented Apr 27, 2021