Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dtslint-runner should not compare against master on DT PRs #897

Open
jakebailey opened this issue Jan 8, 2024 · 0 comments
Open

dtslint-runner should not compare against master on DT PRs #897

jakebailey opened this issue Jan 8, 2024 · 0 comments

Comments

@jakebailey
Copy link
Member

dtslint-runner diffs against master. If someone merges a PR at the exact same time as another PR's CI runs, they'll pull origin/master and may get the wrong thing. For example, in https://github.com/DefinitelyTyped/DefinitelyTyped/actions/runs/7452339628/job/20275379960?pr=68135:

Running: git rev-parse --verify master
Running: git fetch origin master

Running: git branch master FETCH_HEAD

Running: git diff master --name-status
M	types/matter-js/index.d.ts
M	types/slate-html-serializer/index.d.ts
M	types/slate-html-serializer/tsconfig.json
Testing 2 changed packages: Set(2) { 'matter-js', 'slate-html-serializer' }
Testing 0 dependent packages: Set(0) {}

dtslint-runner fetches master, but gets a newer master than the PR, so it believes that the PR modified matter-js, when it was actually another PR merged to master. I partially fixed this for pnpm install in https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/scripts/pnpm-install.sh; since PRs are run on merge commits in CI, HEAD^1 points to an already pulled / stable merge base (what the PR thinks master is at the time of the CI run; further runs will get newer merge bases so it's not out of date).

This then causes further failures because pnpm install is installing the "correct" dependency set, which does not include matter-js.

In CI, it's easy to tell that we're in a PR. But, I'm not totally sure of the right mechanism to check in dtslint-runner, but we should definitely do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant