From 3e96d6309b75ea86afcf4674671ac58cc3e6a45b Mon Sep 17 00:00:00 2001 From: "James M. Greene" Date: Tue, 9 Mar 2021 10:18:45 -0600 Subject: [PATCH 1/6] Revert repo-sync to its old auto-merge strategy (#18189) --- .github/workflows/repo-sync.yml | 30 +----------------------------- 1 file changed, 1 insertion(+), 29 deletions(-) diff --git a/.github/workflows/repo-sync.yml b/.github/workflows/repo-sync.yml index 1e08ecd3c9..cbc20bbc53 100644 --- a/.github/workflows/repo-sync.yml +++ b/.github/workflows/repo-sync.yml @@ -40,7 +40,7 @@ jobs: destination_branch: main pr_title: 'repo sync' pr_body: "This is an automated pull request to sync changes between the public and private repos.\n\n:robot: This pull request should be merged (not squashed) to preserve continuity across repos, so please let a bot do the merging!" - pr_label: autoupdate,automated-reposync-pr + pr_label: automerge,autoupdate,automated-reposync-pr github_token: ${{ secrets.OCTOMERGER_PAT_WITH_REPO_AND_WORKFLOW_SCOPE }} - name: Find pull request @@ -88,34 +88,6 @@ jobs: console.log(`Branch is already up-to-date`) } - - name: Enable GitHub auto-merge - if: ${{ steps.find-pull-request.outputs.number }} - uses: actions/github-script@626af12fe9a53dc2972b48385e7fe7dec79145c9 - with: - github-token: ${{ secrets.GITHUB_TOKEN }} - script: | - const pull = await github.pulls.get({ - ...context.repo, - pull_number: parseInt(${{ steps.find-pull-request.outputs.number }}) - }) - - const pullNodeId = pull.data.node_id - console.log(`Pull request GraphQL Node ID: ${pullNodeId}`) - - const mutation = `mutation ($id: ID!) { - enablePullRequestAutoMerge(input: { - pullRequestId: $id, - mergeMethod: MERGE - }) { - clientMutationId - } - }` - const variables = { - id: pullNodeId - } - await github.graphql(mutation, variables) - console.log('Auto-merge enabled!') - - name: Send Slack notification if workflow fails uses: someimportantcompany/github-actions-slack-message@0b470c14b39da4260ed9e3f9a4f1298a74ccdefd if: failure() From 1d87a7e10c7769fc2f22b21aed3dac27a404e9e1 Mon Sep 17 00:00:00 2001 From: Ethan Palm <56270045+ethanpalm@users.noreply.github.com> Date: Tue, 9 Mar 2021 12:32:15 -0500 Subject: [PATCH 2/6] Add that Discussions are available in private repos & bug fixes (#18165) Co-authored-by: Melanie Yarbrough <11952755+myarb@users.noreply.github.com> --- ...anaging-categories-for-discussions-in-your-repository.md | 4 +--- .../managing-discussions-in-your-repository.md | 2 +- content/discussions/quickstart.md | 6 +++--- data/reusables/gated-features/discussions.md | 2 +- 4 files changed, 6 insertions(+), 8 deletions(-) diff --git a/content/discussions/managing-discussions-for-your-community/managing-categories-for-discussions-in-your-repository.md b/content/discussions/managing-discussions-for-your-community/managing-categories-for-discussions-in-your-repository.md index 10a420e0db..56cc724cd5 100644 --- a/content/discussions/managing-discussions-for-your-community/managing-categories-for-discussions-in-your-repository.md +++ b/content/discussions/managing-discussions-for-your-community/managing-categories-for-discussions-in-your-repository.md @@ -12,9 +12,7 @@ versions: {% data reusables.discussions.about-discussions %} {% data reusables.discussions.about-categories-and-formats %} -Each category must have a unique name and emoji pairing, and can be accompanied by a detailed description stating its purpose. Categories help maintainers organize how conversations are filed and are customizable to help distinguish categories that are Q&A or more open-ended conversations.{% data reusables.discussions.repository-category-limit %} - -For more information, see "[About discussions](/discussions/collaborating-with-your-community-using-discussions/about-discussions#about-categories-and-formats-for-discussions)." +Each category must have a unique name and emoji pairing, and can be accompanied by a detailed description stating its purpose. Categories help maintainers organize how conversations are filed and are customizable to help distinguish categories that are Q&A or more open-ended conversations. {% data reusables.discussions.repository-category-limit %} For more information, see "[About discussions](/discussions/collaborating-with-your-community-using-discussions/about-discussions#about-categories-and-formats-for-discussions)." ### Default categories diff --git a/content/discussions/managing-discussions-for-your-community/managing-discussions-in-your-repository.md b/content/discussions/managing-discussions-for-your-community/managing-discussions-in-your-repository.md index 28251d0478..230201e4d9 100644 --- a/content/discussions/managing-discussions-for-your-community/managing-discussions-in-your-repository.md +++ b/content/discussions/managing-discussions-for-your-community/managing-discussions-in-your-repository.md @@ -24,7 +24,7 @@ To manage discussions in a repository, discussions must be enabled for the repos ### Changing the category for a discussion -You can categorize discussions to help community members find related discussions. For more information, see "[Managing categories for discussions in your repository](/discussions/managing-discussions-for-your-community/managing-categories-for-discussions-in-your-repository)" article. +You can categorize discussions to help community members find related discussions. For more information, see "[Managing categories for discussions in your repository](/discussions/managing-discussions-for-your-community/managing-categories-for-discussions-in-your-repository)." You can also move a discussion to a different category. diff --git a/content/discussions/quickstart.md b/content/discussions/quickstart.md index 252da3b6cc..4609b2fddb 100644 --- a/content/discussions/quickstart.md +++ b/content/discussions/quickstart.md @@ -16,7 +16,7 @@ Discussions give a space for more collaborative conversations by connecting and ### Enabling {% data variables.product.prodname_discussions %} on your repository -Repository owners and people with write access can enable {% data variables.product.prodname_discussions %} for a community on their public repositories. +Repository owners and people with write access can enable {% data variables.product.prodname_discussions %} for a community on their public and private repositories. When you first enable a {% data variables.product.prodname_discussions %}, you will be invited to configure a welcome post. @@ -49,13 +49,13 @@ Anyone with access to a repository can create a discussion. ### Organizing discussions into relevant categories -Repository owners and people with write access can create new categories to keep discussions organized. Collaborators participating and creating new discussions can group discussions into the most relevant existing categories. Discussions can also be recategorized after they are created. For more information, see "[Managing categories for discussions in your repository](/discussions/managing-discussions-for-your-community/managing-categories-for-discussions-in-your-repository)" +Repository owners and people with write access can create new categories to keep discussions organized. Collaborators participating and creating new discussions can group discussions into the most relevant existing categories. Discussions can also be recategorized after they are created. For more information, see "[Managing categories for discussions in your repository](/discussions/managing-discussions-for-your-community/managing-categories-for-discussions-in-your-repository)." ### Promoting healthy conversations People with write permissions for a repository can help surface important conversations by pinning discussions, deleting discussions that are no longer useful or are damaging to the community, and transferring discussions to more relevant repositories owned by the organization. For more information, see "[Managing discussions in your repository](/discussions/managing-discussions-for-your-community/managing-discussions-in-your-repository)." -People with triage permissions for a repository can help moderate a project's discussions by marking comments as answers, locking discussions that are not longer useful or are damaging to the community, and converting issues to discussions when an idea is still in the early stages of development. For more information, see "[Moderating discussions](/discussions/managing-discussions-for-your-community/moderating-discussions)." +People with triage permissions for a repository can help moderate a project's discussions by marking comments as answers, locking discussions that are no longer useful or are damaging to the community, and converting issues to discussions when an idea is still in the early stages of development. For more information, see "[Moderating discussions](/discussions/managing-discussions-for-your-community/moderating-discussions)." ### Next steps diff --git a/data/reusables/gated-features/discussions.md b/data/reusables/gated-features/discussions.md index 5ada0a25ee..860427abe2 100644 --- a/data/reusables/gated-features/discussions.md +++ b/data/reusables/gated-features/discussions.md @@ -1 +1 @@ -{% data variables.product.prodname_discussions %} is available in beta for public repositories on {% data variables.product.prodname_dotcom_the_website %}. {% data reusables.gated-features.more-info-org-products %} +{% data variables.product.prodname_discussions %} is available in beta for public and private repositories on {% data variables.product.prodname_dotcom_the_website %}. {% data reusables.gated-features.more-info-org-products %} From af7c0553b661c46b178f1d78d989f9cd1556539b Mon Sep 17 00:00:00 2001 From: Matthew Isabel Date: Tue, 9 Mar 2021 13:12:44 -0500 Subject: [PATCH 3/6] Changes to devcontainer for codespaces (#17805) * Changes to devcontainer for codespaces * Still run `npm start` Co-authored-by: James M. Greene --- .devcontainer.json | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.devcontainer.json b/.devcontainer.json index 10a610b4e7..86443217a1 100644 --- a/.devcontainer.json +++ b/.devcontainer.json @@ -2,13 +2,12 @@ // For format details, see https://aka.ms/vscode-remote/devcontainer.json { "name": "docs.github.com", - "service": "container-doc", "settings": { "terminal.integrated.shell.linux": "/bin/bash", "cSpell.language": ",en" }, // Install pre-requisites, and start to serve docs.github.com locally - "postCreateCommand": "npm install && npm start", + "postCreateCommand": "npm ci && npm run build && npm start", "forwardPorts": [4000], // Visual Studio Code extensions which help authoring for docs.github.com. "extensions": [ From 1ee4d3f670be17fa8260813889f8f5981ae879db Mon Sep 17 00:00:00 2001 From: "James M. Greene" Date: Tue, 9 Mar 2021 13:07:38 -0600 Subject: [PATCH 4/6] Remove `npm start` from Codespaces postCreateCommand (#18193) --- .devcontainer.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.devcontainer.json b/.devcontainer.json index 86443217a1..c2812d7b6b 100644 --- a/.devcontainer.json +++ b/.devcontainer.json @@ -6,8 +6,8 @@ "terminal.integrated.shell.linux": "/bin/bash", "cSpell.language": ",en" }, - // Install pre-requisites, and start to serve docs.github.com locally - "postCreateCommand": "npm ci && npm run build && npm start", + // Install pre-requisites and run a build to ensure we are ready to start serving docs.github.com locally (via `npm start`) + "postCreateCommand": "npm ci && npm run build", "forwardPorts": [4000], // Visual Studio Code extensions which help authoring for docs.github.com. "extensions": [ From fd7d0eeb1a1c27ccbcb9a942aee360c705d607e5 Mon Sep 17 00:00:00 2001 From: "James M. Greene" Date: Tue, 9 Mar 2021 13:14:02 -0600 Subject: [PATCH 5/6] Add timeout and abort middleware and processing halts (#18177) * Add middleware to timeout requests after a period * Add halt-on-dropped-connection middleware to stop the middleware processing stack if the connection was already dropped * Add a few strategic bail-out spots for dropped connections during the render-page middleware * Handle 404s and HEAD requests earlier in the page rendering flow * Add a few more strategic bail-out spots for dropped connections during the render-page middleware * Add middleware to notice aborted requests * Add a check for aborted requests into the isConnectionDropped logic * Reformat comment for consistency * Handle aborted requests correctly in the error handler * Explicit returns for consistency --- middleware/abort.js | 15 ++++++ middleware/halt-on-dropped-connection.js | 18 +++++++ middleware/handle-errors.js | 8 +-- middleware/index.js | 18 ++++++- middleware/render-page.js | 63 ++++++++++++++---------- middleware/timeout.js | 27 ++++++++++ package-lock.json | 5 ++ package.json | 1 + 8 files changed, 126 insertions(+), 29 deletions(-) create mode 100644 middleware/abort.js create mode 100644 middleware/halt-on-dropped-connection.js create mode 100644 middleware/timeout.js diff --git a/middleware/abort.js b/middleware/abort.js new file mode 100644 index 0000000000..a7351c29bf --- /dev/null +++ b/middleware/abort.js @@ -0,0 +1,15 @@ +module.exports = function (req, res, next) { + // If the client aborts the connection, send an error + req.once('aborted', () => { + // NOTE: Node.js will also automatically set `req.aborted = true` + + const abortError = new Error('Client closed request') + abortError.statusCode = 499 + abortError.code = 'ECONNRESET' + + // Pass the error to the Express error handler + return next(abortError) + }) + + return next() +} diff --git a/middleware/halt-on-dropped-connection.js b/middleware/halt-on-dropped-connection.js new file mode 100644 index 0000000000..9745723a84 --- /dev/null +++ b/middleware/halt-on-dropped-connection.js @@ -0,0 +1,18 @@ +function isConnectionDropped (req, res) { + // Have the flags been set for: + // - a global request timeout (via the express-timeout-handler middleware)? + // - an aborted request connection (via Node.js core's HTTP IncomingMessage)? + return Boolean(res.globalTimeout || req.aborted) +} + +function haltOnDroppedConnection (req, res, next) { + // Only proceed if the flag has not been set for the express-timeout-handler middleware + if (!isConnectionDropped(req, res)) { + return next() + } +} + +// Export this logic, too +haltOnDroppedConnection.isConnectionDropped = isConnectionDropped + +module.exports = haltOnDroppedConnection diff --git a/middleware/handle-errors.js b/middleware/handle-errors.js index 7e3757ae94..7fbb456ab9 100644 --- a/middleware/handle-errors.js +++ b/middleware/handle-errors.js @@ -6,7 +6,9 @@ const loadSiteData = require('../lib/site-data') function shouldLogException (error) { const IGNORED_ERRORS = [ // avoid sending CSRF token errors (from bad-actor POST requests) - 'EBADCSRFTOKEN' + 'EBADCSRFTOKEN', + // Client connected aborted + 'ECONNRESET' ] if (IGNORED_ERRORS.includes(error.code)) { @@ -26,8 +28,8 @@ async function logException (error, req) { } module.exports = async function handleError (error, req, res, next) { - // If the headers have already been sent... - if (res.headersSent) { + // If the headers have already been sent or the request was aborted... + if (res.headersSent || req.aborted) { // Report to Failbot await logException(error, req) diff --git a/middleware/index.js b/middleware/index.js index 6d39616432..77c99177fa 100644 --- a/middleware/index.js +++ b/middleware/index.js @@ -1,5 +1,6 @@ const express = require('express') const instrument = require('../lib/instrument-middleware') +const haltOnDroppedConnection = require('./halt-on-dropped-connection') const isDevelopment = process.env.NODE_ENV === 'development' @@ -11,6 +12,10 @@ const asyncMiddleware = fn => } module.exports = function (app) { + // *** Request connection management *** + app.use(require('./timeout')) + app.use(require('./abort')) + // *** Development tools *** app.use(require('morgan')('dev', { skip: (req, res) => !isDevelopment })) if (isDevelopment) app.use(require('./webpack')) @@ -60,6 +65,9 @@ module.exports = function (app) { app.use(instrument('./find-page')) // Must come before archived-enterprise-versions, breadcrumbs, featured-links, products, render-page app.use(instrument('./block-robots')) + // Check for a dropped connection before proceeding + app.use(haltOnDroppedConnection) + // *** Rendering, 2xx responses *** // I largely ordered these by use frequency app.use(instrument('./archived-enterprise-versions-assets')) // Must come before static/assets @@ -91,6 +99,9 @@ module.exports = function (app) { app.use(instrument('./loaderio-verification')) app.get('/_500', asyncMiddleware(instrument('./trigger-error'))) + // Check for a dropped connection before proceeding (again) + app.use(haltOnDroppedConnection) + // *** Preparation for render-page *** app.use(asyncMiddleware(instrument('./contextualizers/enterprise-release-notes'))) app.use(instrument('./contextualizers/graphql')) @@ -106,7 +117,12 @@ module.exports = function (app) { // *** Headers for pages only *** app.use(require('./set-fastly-cache-headers')) - // *** Rendering, must go last *** + // Check for a dropped connection before proceeding (again) + app.use(haltOnDroppedConnection) + + // *** Rendering, must go almost last *** app.get('/*', asyncMiddleware(instrument('./render-page'))) + + // *** Error handling, must go last *** app.use(require('./handle-errors')) } diff --git a/middleware/render-page.js b/middleware/render-page.js index 1ee5b3d161..0bb4e62774 100644 --- a/middleware/render-page.js +++ b/middleware/render-page.js @@ -6,6 +6,7 @@ const getMiniTocItems = require('../lib/get-mini-toc-items') const Page = require('../lib/page') const statsd = require('../lib/statsd') const RedisAccessor = require('../lib/redis-accessor') +const { isConnectionDropped } = require('./halt-on-dropped-connection') const { HEROKU_RELEASE_VERSION } = process.env const pageCacheDatabaseNumber = 1 @@ -28,31 +29,6 @@ function addCsrf (req, text) { module.exports = async function renderPage (req, res, next) { const page = req.context.page - // Remove any query string (?...) and/or fragment identifier (#...) - const { pathname, searchParams } = new URL(req.originalUrl, 'https://docs.github.com') - - for (const queryKey in req.query) { - if (!cacheableQueries.includes(queryKey)) { - searchParams.delete(queryKey) - } - } - const originalUrl = pathname + ([...searchParams].length > 0 ? `?${searchParams}` : '') - - // Serve from the cache if possible (skip during tests) - const isCacheable = !process.env.CI && process.env.NODE_ENV !== 'test' && req.method === 'GET' - - // Is the request for JSON debugging info? - const isRequestingJsonForDebugging = 'json' in req.query && process.env.NODE_ENV !== 'production' - - if (isCacheable && !isRequestingJsonForDebugging) { - const cachedHtml = await pageCache.get(originalUrl) - if (cachedHtml) { - console.log(`Serving from cached version of ${originalUrl}`) - statsd.increment('page.sent_from_cache') - return res.send(addCsrf(req, cachedHtml)) - } - } - // render a 404 page if (!page) { if (process.env.NODE_ENV !== 'test' && req.context.redirectNotFound) { @@ -70,15 +46,52 @@ module.exports = async function renderPage (req, res, next) { return res.status(200).end() } + // Remove any query string (?...) and/or fragment identifier (#...) + const { pathname, searchParams } = new URL(req.originalUrl, 'https://docs.github.com') + + for (const queryKey in req.query) { + if (!cacheableQueries.includes(queryKey)) { + searchParams.delete(queryKey) + } + } + const originalUrl = pathname + ([...searchParams].length > 0 ? `?${searchParams}` : '') + + // Serve from the cache if possible (skip during tests) + const isCacheable = !process.env.CI && process.env.NODE_ENV !== 'test' && req.method === 'GET' + + // Is the request for JSON debugging info? + const isRequestingJsonForDebugging = 'json' in req.query && process.env.NODE_ENV !== 'production' + + if (isCacheable && !isRequestingJsonForDebugging) { + // Stop processing if the connection was already dropped + if (isConnectionDropped(req, res)) return + + const cachedHtml = await pageCache.get(originalUrl) + if (cachedHtml) { + // Stop processing if the connection was already dropped + if (isConnectionDropped(req, res)) return + + console.log(`Serving from cached version of ${originalUrl}`) + statsd.increment('page.sent_from_cache') + return res.send(addCsrf(req, cachedHtml)) + } + } + // add page context const context = Object.assign({}, req.context, { page }) // collect URLs for variants of this page in all languages context.page.languageVariants = Page.getLanguageVariants(req.path) + // Stop processing if the connection was already dropped + if (isConnectionDropped(req, res)) return + // render page context.renderedPage = await page.render(context) + // Stop processing if the connection was already dropped + if (isConnectionDropped(req, res)) return + // get mini TOC items on articles if (page.showMiniToc) { context.miniTocItems = getMiniTocItems(context.renderedPage, page.miniTocMaxHeadingLevel) diff --git a/middleware/timeout.js b/middleware/timeout.js new file mode 100644 index 0000000000..b8190719ed --- /dev/null +++ b/middleware/timeout.js @@ -0,0 +1,27 @@ +const timeout = require('express-timeout-handler') + +// Heroku router requests timeout after 30 seconds. We should stop them earlier! +const maxRequestTimeout = parseInt(process.env.REQUEST_TIMEOUT, 10) || 25000 + +module.exports = timeout.handler({ + // Default timeout for all endpoints + // To override for a given router/endpoint, use `require('express-timeout-handler').set(...)` + timeout: maxRequestTimeout, + + // IMPORTANT: + // We cannot allow the middleware to disable the `res` object's methods like + // it does by default if we want to use `next` in the `onTimeout` handler! + disable: [], + + onTimeout: function (req, res, next) { + // Create a custom timeout error + const timeoutError = new Error('Request timed out') + timeoutError.statusCode = 503 + timeoutError.code = 'ETIMEDOUT' + + // Pass the error to our Express error handler for consolidated processing + return next(timeoutError) + } + + // Can also set an `onDelayedResponse` property IF AND ONLY IF you allow for disabling methods +}) diff --git a/package-lock.json b/package-lock.json index 847d8365d3..1f5f0dc58b 100644 --- a/package-lock.json +++ b/package-lock.json @@ -10914,6 +10914,11 @@ "resolved": "https://registry.npmjs.org/express-rate-limit/-/express-rate-limit-5.1.3.tgz", "integrity": "sha512-TINcxve5510pXj4n9/1AMupkj3iWxl3JuZaWhCdYDlZeoCPqweGZrxbrlqTCFb1CT5wli7s8e2SH/Qz2c9GorA==" }, + "express-timeout-handler": { + "version": "2.2.0", + "resolved": "https://registry.npmjs.org/express-timeout-handler/-/express-timeout-handler-2.2.0.tgz", + "integrity": "sha512-Ae9M32r3T+RmrY1MGwBvyVjnzVo1iD4s0YQHgvJqzPlT0d4qS84pr87LHyiUtnD7qDQLMYw6rl2fhsgH4TH8aQ==" + }, "ext": { "version": "1.4.0", "resolved": "https://registry.npmjs.org/ext/-/ext-1.4.0.tgz", diff --git a/package.json b/package.json index 008ddf8da0..967d8253d9 100644 --- a/package.json +++ b/package.json @@ -43,6 +43,7 @@ "dotenv": "^8.2.0", "express": "^4.17.1", "express-rate-limit": "^5.1.3", + "express-timeout-handler": "^2.2.0", "flat": "^5.0.0", "github-slugger": "^1.2.1", "got": "^9.6.0", From c76a357b599a717f8d0415f3a24b7ac6e2af5621 Mon Sep 17 00:00:00 2001 From: Rachael Sewell Date: Tue, 9 Mar 2021 17:03:57 -0800 Subject: [PATCH 6/6] Revert "run update branch on a cron (#18138)" (#18203) This reverts commit e95c986581c3f6d6a6aebac5000083c24c3c9f8c. --- .github/workflows/autoupdate-branch.yml | 2 -- 1 file changed, 2 deletions(-) diff --git a/.github/workflows/autoupdate-branch.yml b/.github/workflows/autoupdate-branch.yml index 0e0729da46..c1e5b92b42 100644 --- a/.github/workflows/autoupdate-branch.yml +++ b/.github/workflows/autoupdate-branch.yml @@ -17,8 +17,6 @@ on: push: branches: - main - schedule: - - cron: '*/30 * * * *' # every 30 minutes jobs: autoupdate: