bitsbeats / drone-tree-config

Drone helper for mono repositories.
Apache License 2.0
103 stars 24 forks source link

Multiple commits #7

Closed gevalter closed 4 years ago

gevalter commented 4 years ago

Hey guys, your plugin worked great for me until one of our programmers did the following use case:

The plugin suppose to go over all commits or only the last one?

foosinn commented 4 years ago

Drone does not always provide a last commit. Was the branch created with the push?

When drone provides no last build, drone-tree-config asumes just the last one. If you open a pull request the tests should be fine.

gevalter commented 4 years ago

Yes, the branch was created with the push after the last commit. And you are right, pull request & Merge builds do solve the problem but we do some test on the branch level before pushing to master.

Maybe if I change this part to take the master branch as the before variable it would work? Or you think it will break something else?

// use diff to get changed files before := req.Build.Before if before == "0000000000000000000000000000000000000000" || before == "" { before = fmt.Sprintf("%s~1", req.Build.After) } var err error changedFiles, err = req.Client.ChangedFilesInDiff(ctx, before, req.Build.After)

foosinn commented 4 years ago

i do not think its save to assume that the default branch is master.

also as far as i remember there where at least occasions where this happened

currently i do not have the time for a full test setup. can you investigate further?

gevalter commented 4 years ago

Hey, I found the bug and fixed it for our use case, i also edit the tests module, added the values needed for my solution to work, I opened a PR for this changes.

The problem was for builds triggered from push event, there was a git-diff between the last commit and the one before, that is wrong - when you push to remote branch in a multi branch environment - sometimes you have more than 1 commit.

scm.go: } else { // use diff to get changed files before := req.Build.Before if before == "0000000000000000000000000000000000000000" || before == "" { before = fmt.Sprintf("%s~1", req.Build.After) }

There are 2 cases -

  1. New branch - condition is true, the "before" variable is populated with the last commit with a suffix of "~1", (which i don't understand why you inserted the suffix), and the "after" variable is also the last commit.
  2. Existing branch - condition is false, the "before" variable is populated with the commit before the push event and the "after" variable is also the last commit.
foosinn commented 4 years ago

PR has been merged.