Add a whatif command to find out the impact of changing a module about mbt HOT 16 CLOSED

@BuddySpike would it make sense for the whatif command to follow the CLI style for the other commands (e.g. whatif <local|branch|...etc> <...modules-list>) ? Or should it just test against local changes (e.g. whatif <...modules-list>) ?

from mbt.

Thanks for the clarification.

You are right, this is mostly useful in the context of local or head.
It could also be useful for branch - say you might want to find out what impact a change to module A would have in a particular branch without switching to it.
Similarly, on a large diff, to narrow down an impact of a particular module (for example, while trying to diagnose a build)

Thoughts?

from mbt.

Hmm yea I didn't think of the other use cases for branch or on a large diff, thanks for pointing them out. Okk great, yea it seems to be correct to have it in all contexts then since it has it proper use in all 😃

from mbt.

Awesome, just created this #115 to track it.

from mbt.

@ojizero That's a fantastic idea.

This issue is so old 😉 and I'm wondering if we could achieve the same thing by modifying the behaviour of describe command? describe currently support filtering with --name argument. What it does not do however is displaying the dependents of filtered modules. I wonder if it would now make more sense to add --dependants switch to make it expand path.

The idea behind this feature is to be able to do a quick impact assessment. And describe command already support visualising the graph (via graphviz tools) and could be a handy extension.

What do you think?

from mbt.

@BuddySpike yea I too think it could make more sense to have this feature be a part of the describe command, that way would also help having a singular command as a one stop shop for all information gathering about the repo and it's modules instead of having multiple commands to do so.

I think I can start working on implementing it starting tonight.

Do you have any recommendations regarding it's expected behaviour (for example ordering or grouping the output by the the module that caused the effect) or would such behaviour be not needed?

from mbt.

Thanks @ojizero this would be great!

I think we can keep the console output simple because trying to represent this in a line based stdout would require some thought (I think 😄)

Users who are after a more complex representation can always generate the graphviz visualisation by piping the output you get when you use describe with --graph option.

I think it's worth validating the current behaviour of both console and graph output and see where the gap is.

Also, we can augment the generated graphviz dot format with different colors etc to make the effect more understandable (this is done in https://github.com/mbtproject/mbt/blob/master/lib/dot_serializer.go).

Thoughts?

from mbt.

Actually, we could also introduce --svg flag to describe which will basically run graphviz dot command under the hood for you. I've seen another utility doing this but can't recall which one right now.

from mbt.

@BuddySpike I've been looking at the describe source and thinking about it's behaviour/usage.

When running describe diff for example it represents what modules have gotten changed between two commits correct? Wouldn't it be strange or odd, to have an option --dependents when the diff would already output those as changed anyway (unless you filter with --name)?

I'm not sure if I'm making my point clear but wouldn't it make more sense for the --dependents flag to be available when running the describe local only? Since it only makes sense to see the potential impact of a change on the current workspace (repo revision)? That is, is it a valid case to test for an impact I could change on the repo I have locally, but instead to run the say describe diff or describe pr versions, or is it only valid for the describe local variant of the command ?

from mbt.

Yeah, it does not make sense to have --dependents flag without --name I guess. If we make it a complementary flag for --name, I think it would make sense in all contexts. What do you think?

from mbt.

I think I didn't communicate my thought correctly 😅

What I'm thinking of is the usage of such flag (the whatif behaviour itself), mainly that I want to know, that if I make a change to module A what other modules could potentially be impacted by this possible change.

My though is that this question "What is impacted by my potential change to module A?", does it apply to any context aside from the local one? i.e. does make sense to make this question when working on say the diff or branch contexts or not? Since if you are asking this question, you're maybe assessing the complexity of releasing some change, meaning you're most probably working from within the current local version of the code, which you have setup now.

I hope I'm clearer now 😅

from mbt.

@BuddySpike not sure if I should ask about this here, but I'll do since you mentioned the serializer here anyway 😅

I noticed that in the dot_serializer implementation, you only output the dependencies of one level up of the module if filtered, for example if we have A -> B -> C and a list of modules mods := Modules{A}, then calling the SerializeAsDot method would produce the graph

digraph mbt {
  node [shape=box fillcolor=powderblue style=filled fontcolor=black];
  "A" -> "B"
}

which is only partially true, given that it misses that B -> C part of the graph and should rather be

digraph mbt {
  node [shape=box fillcolor=powderblue style=filled fontcolor=black];
  "A" -> "B"
  "B" -> "C"
}

in order to be more descriptive, or informative of the actual graph, to the user describing a set of modules in a repo.

Is this by design? Or just unintended side-effect of the implementation?

from mbt.

@ojizero This is an interesting find. I would like to call this a bug. However, I'm not sure what it should do anyway. Your PR will surely make things better for those use cases with --dependents flag. What should we do for the others? May be assume --dependents if you specify --graph?

@bashiransari Do you have any thoughts on this?

from mbt.

@BuddySpike actually I don't think enabling the dependents flag with graph flag would solve this, dependents would add modules the require the filter placed, what we're missing in the current implementation is the dependencies of the dependencies of the filtered module (it's transitive dependencies).

from mbt.

You are right. I think it would be useful to visualise the entirety of a filtered sub-graph. Do you think we could address this as a separate feature to what's covered in this issue?

from mbt.

Yea it could be addressed as another issue, if you want I could work on it once we finish this feature 😬

from mbt.