WIP: Verify BC breakages on pull requests, by comparing PR against base branch

[Ocramius]

Q	A
Documentation	no
Bugfix	no
BC Break	no
New Feature	yes
RFC	no
QA	yes

[Ocramius]

yuck yuck yuck - would prefer to merge two sets of Tool[], but for now, this will have to do.

[internalsystemerror]

You could do something like:

const tools = [
    ...[bcTool].filter()
    <other tools>
];

Spread new array containing the `bcTool with filter removing null values.

[Ocramius]

Tried that, but .filter() TSC wasn't able to infer Tool[] from it, due to too many dynamic expressions, I guess.

[internalsystemerror]

...([bcTool].filter() as Tool[]) will do that.

[internalsystemerror]

You could do something like:

const tools = [
    ...[bcTool].filter()
    <other tools>
];

Spread new array containing the `bcTool with filter removing null values.

[boesing]

Change looks fine for me.

• Passing git fetch <basebranch> to before_script should work.
  I'd say lets add it from the tool. I would prefer keeping these before_script and after_script things in internal scope for now. No need to open that up for every check.

• Can we probably remove the empty test.env files? I do not see a good reason why we should have a bunch of dead files around when they do not make a difference.

• I would prefer to pass the targetRef via the appConfig where the supported PHP versions of the project are stored, where the information if a lock file is available is stored, etc. This would probably reduce the LoC which have to change to a minimum and we do not need to pass the action through half of the application scope.

[boesing]

Can we pass the hash in the appConfig instead?
It also holds if there is a lockfile so we do not have to check for fileExists(lockFile) in every tool.
I would prefer having a dedicated object/property in the app config for the target branch REF as well.

[boesing]

Don't we get the target branch ref from environment?
https://docs.github.com/en/actions/learn-github-actions/environment-variables#default-environment-variables

• GITHUB_BASE_REF

Do we really need that whole event parsing here or should we just grab the ref from environment instead?

const GITHUB_BASE_REF = process.env["GITHUB_BASE_REF"] || "" should already do the job, not?

[Ocramius]

I can't seem to be able to allocate a chunk of time to this.

It would save me a massive amount of time in code reviews, but debugging the last details (around fetching multiple refs) is not something I'll be able to do soon.

[boesing]

I'll see if I find some time and apply a few things.

I would prefer to avoid the whole event stuff and instead use GITHUB_BASE_REF. This would tighten down the amount of changes here to a minimum and then we can start with an initial PoC.

We could also release a version where we can opt-in to the new job before rolling out for everyone by-default.

i.e. a .laminas-ci.json flag (which can also be used to fully disable the check in case some1 wants to). This will provide us the possibility to have it field tested in some projects for quite a while. Especially the whole "multiple ref" stuff (which I probably did not fully understand right now) could be verified.

In one of the following releases we can then change the default of the .laminas-ci.json flag to true and rollout the feature for everyone by default.

WDYT?

[boesing]

@Ocramius I am unable to push to your fork and thus, can't push my changes.
Could you enable maintainer edits or does the fork has limited access?

[boesing]

Superseded by #226

[Ocramius]

Could you enable maintainer edits or does the fork has limited access?

I generally don't do this due to repo secrets :-)

Anyway, thanks for taking over on this!