Fix danger local with non master base branch #1058
Conversation
| /** What should we compare against? */ | ||
| base?: string | ||
| /** Should we run against current staged changes? */ | ||
| staging?: boolean |
There was a problem hiding this comment.
Wouldn't this add this parameter also to the other commands, and then to their help too?
There was a problem hiding this comment.
It would, but I think that's a feature so that the base comparison branch will work for all commands. This is needed as I mention in #1057 since danger-runner winds up being invoked.
I did suggest another idea in #1057 but there was a comment that the shared argument solution was preferable.
I'm happy to rework to make it more appropriate
There was a problem hiding this comment.
The other commands are pr and ci runner etc.
In those context I think base and staging would not make sense, because you would be running it against a PR, and not a local branch.
The pr command has a specific command too, https://github.com/danger/danger-js/blob/master/source/commands/danger-pr.ts#L93 and in that case it works, then why does this need to be on the SharedCLI instead? (just asking to try to get more context)
There was a problem hiding this comment.
I'm really not familiar with the code base, but I think that's because those options are only needed before invoking danger runner but the base branch is needed after invoking danger runner.
There was a problem hiding this comment.
Isn't the value taken at line 17 and 19, really similarly to what happens in https://github.com/danger/danger-js/blob/master/source/commands/danger-pr.ts#L93 ? (Again is just me trying to understand better)
There was a problem hiding this comment.
Perhaps my analysis in #1057 is wrong, but I believe what is happening is that danger-local winds up invoking danger-runner and passes the base branch argument, but because it's not a shared option, that override is lost.
Note that in danger-pr the options are removed and so isJSON is never passed to danger-runner. The base branch option needs to be interpreted by danger-runner somehow and the shared arguments is the most obvious way to do that.
There was a problem hiding this comment.
I'm not positive about this, but it does seem like the cliArgs are used inside the runner only if the command is not using a platform. If that's the case, then it's only danger-local.ts that needs to send the base branch.
But danger-runner.ts is invoked by danger-pr, danger-ci, and danger-local (and others), so I think that means we need to merge the cliArgs when generating the json. I've updated the PR
ahobson
force-pushed
the
fix-danger-local-main-branch-1057
branch
from
28e06a2
to
5ee6491
Compare
|
Yeah, I think this is the original intent 👍 |
|
Yeah, I think it should be pretty harmless to pass through arbitrary CLI data regardless. Thanks for taking it up @ahobson |
|
Shipped as 10.3.0 because apparantly I can't remember which is patch or minor it seems 👍 |
This fixes #1057