Skip to content

Adds some infra to warn on files which changed in the PR but aren't accounted for #2901

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

Adds some infra to warn on files which changed in the PR but aren't accounted for #2901

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2677049
Adds some infra to warn on files which changed in the PR but aren't a…
orta Jul 22, 2023
02fe069
Do it in both
orta Jul 22, 2023
3fa6b26
Bring back the CI events for uploading preview envs
orta Jul 25, 2023
c4f0918
Update dangerfile.ts
orta Aug 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 7 additions & 22 deletions .github/workflows/CI.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,29 +40,14 @@ jobs:
env:
CI: true

# Deploy preview environment for non-forks
- uses: Azure/static-web-apps-deploy@v1
if: github.event.pull_request.base.repo.id == github.event.pull_request.head.repo.id
with:
azure_static_web_apps_api_token: ${{ secrets.AZURE_STATIC_WEB_APPS_API_TOKEN_PROD }}
repo_token: ${{ secrets.GITHUB_TOKEN }}
action: upload
app_location: site
skip_app_build: true

# Upload site artifact for forks so it can be deployed by a maintainer on-demand
- uses: actions/upload-artifact@v3
if: github.event.pull_request.base.repo.id != github.event.pull_request.head.repo.id
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Er, is this not the code which powers the deploy-preview label? https://github.com/microsoft/TypeScript-Website/blob/b03935a55750acfadc496e5fdf4258730c96cb5f/.github/workflows/deploy-preview.yml

Probably, just the deploy is what needs to be deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, I thought it was doing a more native version of it, will drop next time I get back to the computer

with:
name: site-${{ github.event.number }}
path: site/

# danger for PR builds
- if: github.event_name == 'pull_request' && github.event.pull_request.base.repo.id == github.event.pull_request.head.repo.id
run: "yarn danger ci"
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
PR_DEPLOY_URL_ROOT: ${{ steps.deploy.outputs.static_web_app_url }}
- name: "Run Danger"
run: |
# Exposing this token is safe because the user of it has no other public repositories
# and has no permission to modify this repository. See DefinitelyTyped #62638 for the discussion.
TOKEN='ghp_i5wtj1l2AbpFv3OU96w6R'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even after reading through the comment thread at DefinitelyTyped/DefinitelyTyped#62638, I'm still not a fan of using an unprotected token. Making this token public means anyone could use it to attempt to DDoS GitHub and have it falsely attributed to the DangerBotOSS account, or DoS Danger by artificially using up its rate limit. If making this a secret is not viable for usability reasons, are there any other mechanisms that could be employed to avoid exposing the token?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really sure why this needs a token at all, either; isn't this just a CI check that can fail and print out the files that were forgotten? a la https://github.com/microsoft/TypeScript/blob/main/.github/workflows/ci.yml#L239

I guess because this tries to print a fancy comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because you need a token to make a comment on an issue, yes!

image

Yep, it's a comment because no-one would read a non-failing CI build for things like warnings

I did explore having a central github app danger/danger-js#1126 but it requires giving too much github access to the bot IMO, and I didn't want to centralize that many people's tokens on my spare time

TOKEN+='On3bHOkcV2AmVY6'
DANGER_GITHUB_API_TOKEN=$TOKEN yarn danger ci

windows:
if: github.event.action != 'closed'
Expand Down
13 changes: 10 additions & 3 deletions dangerfile.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,16 @@
// yarn danger pr https://github.com/microsoft/TypeScript-Website/pull/115

import spellcheck from "danger-plugin-spellcheck"

// Blocked on PR deploys, see CI.yml
// import lighthouse from "danger-plugin-lighthouse"
import { warn, danger }from "danger"
import { execSync } from "child_process"

// Spell check all the things
spellcheck({ settings: "microsoft/[email protected]" })

const gitStatus = execSync("git status --porcelain").toString()
if (gitStatus.includes("M")) {
const files = gitStatus.split("\n").filter(f => f.startsWith(" M ")).map(f => f.substr(3))
const linksToChangedFiles = danger.github.utils.fileLinks(files)

warn(`There are un-staged changes to generated files: \n ${linksToChangedFiles}`)
}
2 changes: 1 addition & 1 deletion packages/documentation/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
"license": "MIT",
"version": "1.0.0",
"scripts": {
"build": "echo 'NOOP'",
"build": "yarn create-handbook-nav",
"test": "echo 'NOOP'",
"lint": "node scripts/lintTwoslashErrors.js",
"create-handbook-nav": "node ./scripts/generateTypesForFilesInDocs && node ./scripts/generateDocsNavigationPerLanguage.js",
Expand Down