Pull Request behind the target branch causing issues

[dchambers]

By comparing a PR that may not have been rebased against the latest changes to master against master itself, graphql-inspector emits backwards compatibility warnings for any recently added fields, even though these fields won't be removed by the PR. Although it's possible to work around this by merging the latest master whenever this happens, this is a bit of a faff, especially given that GitHub doesn't provide a button for doing this unless there's a merge conflict.

Are there any tricks that would allow graphql-inspector to avoid showing these red herrings?

[kamilkisiela]

I don't understand why you want to do it. A Pull Request is defined to be merged into master, so you need to know if the PR won't break anything. If you compare it to something different than what's the point?

Could you explain a bit more? Maybe I don't understand something?

[kamilkisiela]

Closing because I did not get any answer.

[dchambers]

I don't understand why you want to do it. A Pull Request is defined to be merged into master, so you need to know if the PR won't break anything. If you compare it to something different than what's the point?

Could you explain a bit more? Maybe I don't understand something?

Apologies @kamilkisiela, I managed to miss your reply here.

Suppose you have two developers, Bob & Sue, both working on the same codebase. They both pull the code at the same time to begin their work -- let's call the commit they start with #c0.

After a day, Sue commits a change where she adds a new field to the schema (Person.age for arguments sake), so that the latest commit on master is now #c1.

Bob then pushes a new feature that does not change the schema whatsoever, and so can't possibly introduce a backwardly incompatible change. However, graphql-inspector compares his commit (which does not yet contain Person.age) against master, which does, and concludes that Bob is breaking backwards compatibility by removing the Person.age field.

Of course, he's doing no such thing, and the problem is only that the latest version of master (containing commit #c1) was not merged into his branch before performing the comparison.

Hopefully that makes more sense now?

[tylercrumpton]

I'll second this request! We've run into this fairly frequently, with the same circumstances that @dchambers described. We would love to see it compare the merged/rebased commit against the defined schema instead of comparing the tip of the branch against the defined schema.

[kamilkisiela]

sounds reasonable, I will work on a solution then :)

[tylercrumpton]

Wonderful, thanks!

[kamilkisiela]

Should be ready soon :)

[kamilkisiela]

I will try to use https://docs.github.com/en/rest/reference/repos#compare-two-commits and somehow apply the patch, then compare. If there are conflicts, Inspector will fail the ci check and notify about it.

[kamilkisiela]

@tylercrumpton @wopian @euvl @dchambers good news :)

#1692

This way on every pull request we get the schema from refs/pull/PR_NUMBER/merge which means it's merged with the target branch. Your scenario should be solved by that change.

[wopian]

@kamilkisiela is this also available in the GitHub Action?

[kamilkisiela]

@wopian not yet but I'm working on it now

[kamilkisiela]

@wopian done, use master and try to run Inspector Action on pull_request event, when it's on push it won't get a PR number. I would recommend to use pull_request on PRs and push for master branch or something like that.

[kamilkisiela]

Feedback welcome!

[tylercrumpton]

Thanks so much! I'll go give it a shot!

[tylercrumpton]

Looks like it's working perfectly! We'll keep that flag turned on in our repo and see how that goes! Thanks, again!

[wopian]

@kamilkisiela thank you! Will add this to my TODO list, this and something on our end ended up making me postpone adding it to our actions at the time.