Add ability to compare between two branches about affectedmoduledetector

[digitalbuddha]

Merged preliminary config. I need to push it down to module granularity next

[michaelcarrano]

I've been slowly working on a similar plugin at work, so thanks for open sourcing this and finishing my work for me! 😆

For instance, what I was working on, the main focus was for CI so my branches would be a remote branch, not local (although I was planning on a way to config local vs remote). Would need to handle a situation where the branch does not exists, is there a fallback or do you error?

If I were to try and spend some time implementing this enhancement, do you have any guidance on what you would like to see for this enhancement? Please don't wait for me to get around to this enhancement as my day to day varies on how much coding I can do.

[chris-mitchell]

Glad to hear this is helpful for you!

We currently only support running on the local branch and will assume the last commit has all the changes to consider. This is a byproduct of how our CI system works, but understand most aren't like this.

If this was to be added, I would expect that we'd want to allow configuration via the extension

And then updating the GitClient to make the appropriate queries into git. findChangedFilesSince returns a list of files which will be used to determine the changeset.

[digitalbuddha]

I'm planning to take this issue up today

[jonnycaley]

I was also thinking of another ability for the plugin: comparing the changes of the current branch to the parent fork point. Would this be ok to implement?

If so, I noticed in the thread above you mentioned to implement a similar extension to this plugin "allow configuration via the extension". Am I along the right lines in thinking of adding a commitCommand config value that defaults to your current implementation but also allows for extension of other commit sha obtaining commands that can be added in the future?

Something like this? https://gist.github.com/jonnycaley/ad196a3a258dfb0e271a4a261a646426

If not, no worries 😄

[digitalbuddha]

Yup makes sense! I've added the base git logic here. It just needs to be enabled if the extension has a prop.

AffectedModuleDetector/affectedmoduledetector/src/main/kotlin/com/dropbox/affectedmoduledetector/GitClient.kt

Line 183 in 54bf063

const val PREV_MERGE_CMD = "git show-branch -a | grep '\\*' | grep -v `git rev-parse --abbrev-ref HEAD` | head -n1 | sed 's/.*\\[\\(.*\\)\\].*/\\1/' | sed 's/[\\^~].*//'"

I can finish impl over holidays unless someone wants to go to ahead and complete it.

[jonnycaley]

What does that command do? I can't work it out from the shell output 😅

[digitalbuddha]

It should be finding the last merge commit. In theory that should be where you last branched

[jonnycaley]

Ahhh, when I pasted that into a shell it doesn't remove every other \ so I was getting confused. 😅

I think that command gets the parent branch (also found here). We should be able to combine it with the current branch and the command git merge-base current parent to find the commit fork point, something like this:

git merge-base $(git rev-parse --abbrev-ref HEAD) $(git show-branch -a | grep '\*' | grep -v `git rev-parse --abbrev-ref HEAD` | head -n1 | sed 's/.*\[\(.*\)\].*/\1/' | sed 's/[\^~].*//')

If that looks ok i'd like to have a crack at it over the next week or so 😄

[digitalbuddha]

Looks great, please do!

[jonnycaley]

I'm running into bad sha1 reference when I try and run the command PREV_MERGE_CMD in the process builder (works fine in a bash shell). Any ideas why and how to get around it?

[jonnycaley]

It seems ProcessBuilder does not use a shell to execute the commands, so grep is not supported. I'll use kotlin string manipulation instead to get the branch name