From c41964f89bddf696fe7fd495b976466fb82f686a Mon Sep 17 00:00:00 2001 From: Tejas Kumar Date: Tue, 18 May 2021 11:41:31 +0200 Subject: [PATCH 01/14] Scaffolder: Enable branch protection on new repo Signed-off-by: Tejas Kumar --- .../src/scaffolder/stages/publish/github.ts | 23 ++++++- .../src/scaffolder/stages/publish/helpers.ts | 61 +++++++++++++++++++ 2 files changed, 81 insertions(+), 3 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/stages/publish/github.ts b/plugins/scaffolder-backend/src/scaffolder/stages/publish/github.ts index 829856ec0c..599cd32337 100644 --- a/plugins/scaffolder-backend/src/scaffolder/stages/publish/github.ts +++ b/plugins/scaffolder-backend/src/scaffolder/stages/publish/github.ts @@ -15,7 +15,10 @@ */ import { PublisherBase, PublisherOptions, PublisherResult } from './types'; -import { initRepoAndPush } from './helpers'; +import { + enableBranchProtectionOnDefaultRepoBranch, + initRepoAndPush, +} from './helpers'; import { GitHubIntegrationConfig, GithubCredentialsProvider, @@ -99,6 +102,19 @@ export class GithubPublisher implements PublisherBase { /\.git$/, '/blob/master/catalog-info.yaml', ); + + try { + await enableBranchProtectionOnDefaultRepoBranch({ + owner, + client, + repoName: name, + }); + } catch (e) { + throw new Error( + `Failed to add branch protection to '${name}': ${e.message}`, + ); + } + return { remoteUrl, catalogInfoUrl }; } @@ -130,7 +146,7 @@ export class GithubPublisher implements PublisherBase { description, }); - const { data } = await repoCreationPromise; + const { data: newRepo } = await repoCreationPromise; try { if (access?.startsWith(`${owner}/`)) { @@ -156,6 +172,7 @@ export class GithubPublisher implements PublisherBase { `Failed to add access to '${access}'. Status ${e.status} ${e.message}`, ); } - return data?.clone_url; + + return newRepo.clone_url; } } diff --git a/plugins/scaffolder-backend/src/scaffolder/stages/publish/helpers.ts b/plugins/scaffolder-backend/src/scaffolder/stages/publish/helpers.ts index ffefb88cf0..6527a30fe5 100644 --- a/plugins/scaffolder-backend/src/scaffolder/stages/publish/helpers.ts +++ b/plugins/scaffolder-backend/src/scaffolder/stages/publish/helpers.ts @@ -17,6 +17,7 @@ import globby from 'globby'; import { Logger } from 'winston'; import { Git } from '@backstage/backend-common'; +import { Octokit, RestEndpointMethodTypes } from '@octokit/rest'; export async function initRepoAndPush({ dir, @@ -67,3 +68,63 @@ export async function initRepoAndPush({ remote: 'origin', }); } + +type BranchProtectionOptions = { + client: Octokit; + owner: string; + repoName: string; + isRetry?: boolean; +}; + +export const enableBranchProtectionOnDefaultRepoBranch = async ({ + repoName, + client, + owner, + isRetry = false, +}: BranchProtectionOptions): Promise< + RestEndpointMethodTypes['repos']['updateBranchProtection']['response'] +> => { + const { data: repo } = await client.repos.get({ + owner, + repo: repoName, + }); + + try { + const response = await client.repos.updateBranchProtection({ + headers: { + Accept: + /** + * 👇 we need this header because allowing a custom + * reviewer count on branch protection is a preview + * feature. + * + * More here: https://docs.github.com/en/rest/overview/api-previews#require-multiple-approving-reviews + */ + 'application/vnd.github.luke-cage-preview+json', + }, + owner, + repo: repoName, + branch: repo.default_branch, + required_status_checks: { strict: true, contexts: [] }, + restrictions: null, + enforce_admins: true, + required_pull_request_reviews: { required_approving_review_count: 1 }, + }); + + return response; + } catch (e) { + if (!isRetry && e.message.includes('Branch not found')) { + // GitHub has eventual consistency. Fail silently, wait, and try again. + await new Promise(resolve => setTimeout(resolve, 600)); + + return await enableBranchProtectionOnDefaultRepoBranch({ + repoName, + client, + owner, + isRetry: true, + }); + } + + throw e; + } +}; From 9962faa2b0a78e764f7c013fcb89beef0e58cb69 Mon Sep 17 00:00:00 2001 From: Tejas Kumar Date: Tue, 18 May 2021 11:47:05 +0200 Subject: [PATCH 02/14] Add changeset Signed-off-by: Tejas Kumar --- .changeset/thick-ads-smash.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/thick-ads-smash.md diff --git a/.changeset/thick-ads-smash.md b/.changeset/thick-ads-smash.md new file mode 100644 index 0000000000..68f592269f --- /dev/null +++ b/.changeset/thick-ads-smash.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-scaffolder-backend': minor +--- + +Add branch protection for default branches of scaffolded GitHub repositories From 46bb4a1482d78e9bcfdf68a096de2eb418b3273c Mon Sep 17 00:00:00 2001 From: Tejas Kumar Date: Tue, 18 May 2021 13:31:00 +0200 Subject: [PATCH 03/14] Adjust error output Signed-off-by: Tejas Kumar --- .../src/scaffolder/stages/publish/github.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/stages/publish/github.ts b/plugins/scaffolder-backend/src/scaffolder/stages/publish/github.ts index 599cd32337..c7b7076095 100644 --- a/plugins/scaffolder-backend/src/scaffolder/stages/publish/github.ts +++ b/plugins/scaffolder-backend/src/scaffolder/stages/publish/github.ts @@ -110,9 +110,7 @@ export class GithubPublisher implements PublisherBase { repoName: name, }); } catch (e) { - throw new Error( - `Failed to add branch protection to '${name}': ${e.message}`, - ); + throw new Error(`Failed to add branch protection to '${name}', ${e}`); } return { remoteUrl, catalogInfoUrl }; From 9dce89d9ba3d33439b6d776da7b7b993ceef2b3e Mon Sep 17 00:00:00 2001 From: Tejas Kumar Date: Tue, 18 May 2021 13:31:55 +0200 Subject: [PATCH 04/14] Clean up return type Signed-off-by: Tejas Kumar --- .../src/scaffolder/stages/publish/helpers.ts | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/stages/publish/helpers.ts b/plugins/scaffolder-backend/src/scaffolder/stages/publish/helpers.ts index 6527a30fe5..dcf444fa6c 100644 --- a/plugins/scaffolder-backend/src/scaffolder/stages/publish/helpers.ts +++ b/plugins/scaffolder-backend/src/scaffolder/stages/publish/helpers.ts @@ -17,7 +17,7 @@ import globby from 'globby'; import { Logger } from 'winston'; import { Git } from '@backstage/backend-common'; -import { Octokit, RestEndpointMethodTypes } from '@octokit/rest'; +import { Octokit } from '@octokit/rest'; export async function initRepoAndPush({ dir, @@ -81,16 +81,14 @@ export const enableBranchProtectionOnDefaultRepoBranch = async ({ client, owner, isRetry = false, -}: BranchProtectionOptions): Promise< - RestEndpointMethodTypes['repos']['updateBranchProtection']['response'] -> => { +}: BranchProtectionOptions): Promise => { const { data: repo } = await client.repos.get({ owner, repo: repoName, }); try { - const response = await client.repos.updateBranchProtection({ + await client.repos.updateBranchProtection({ headers: { Accept: /** @@ -110,19 +108,19 @@ export const enableBranchProtectionOnDefaultRepoBranch = async ({ enforce_admins: true, required_pull_request_reviews: { required_approving_review_count: 1 }, }); - - return response; } catch (e) { if (!isRetry && e.message.includes('Branch not found')) { // GitHub has eventual consistency. Fail silently, wait, and try again. await new Promise(resolve => setTimeout(resolve, 600)); - return await enableBranchProtectionOnDefaultRepoBranch({ + await enableBranchProtectionOnDefaultRepoBranch({ repoName, client, owner, isRetry: true, }); + + return; } throw e; From 16cdf5cda2c796da2656888f6816317c180e2b7a Mon Sep 17 00:00:00 2001 From: Tejas Kumar Date: Tue, 18 May 2021 13:40:02 +0200 Subject: [PATCH 05/14] Update version bump Signed-off-by: Tejas Kumar --- .changeset/thick-ads-smash.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/thick-ads-smash.md b/.changeset/thick-ads-smash.md index 68f592269f..ef4881f759 100644 --- a/.changeset/thick-ads-smash.md +++ b/.changeset/thick-ads-smash.md @@ -1,5 +1,5 @@ --- -'@backstage/plugin-scaffolder-backend': minor +'@backstage/plugin-scaffolder-backend': patch --- Add branch protection for default branches of scaffolded GitHub repositories From c2cfac4cbe6f419dc6870a08479f3a275dd8bdb0 Mon Sep 17 00:00:00 2001 From: Tejas Kumar Date: Tue, 18 May 2021 17:06:35 +0200 Subject: [PATCH 06/14] Use Octokit's previews API Signed-off-by: Tejas Kumar --- .../src/scaffolder/stages/publish/helpers.ts | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/stages/publish/helpers.ts b/plugins/scaffolder-backend/src/scaffolder/stages/publish/helpers.ts index dcf444fa6c..9d5effa8b2 100644 --- a/plugins/scaffolder-backend/src/scaffolder/stages/publish/helpers.ts +++ b/plugins/scaffolder-backend/src/scaffolder/stages/publish/helpers.ts @@ -89,16 +89,15 @@ export const enableBranchProtectionOnDefaultRepoBranch = async ({ try { await client.repos.updateBranchProtection({ - headers: { - Accept: - /** - * 👇 we need this header because allowing a custom - * reviewer count on branch protection is a preview - * feature. - * - * More here: https://docs.github.com/en/rest/overview/api-previews#require-multiple-approving-reviews - */ - 'application/vnd.github.luke-cage-preview+json', + mediaType: { + /** + * 👇 we need this preview because allowing a custom + * reviewer count on branch protection is a preview + * feature. + * + * More here: https://docs.github.com/en/rest/overview/api-previews#require-multiple-approving-reviews + */ + previews: ['luke-cage-preview'], }, owner, repo: repoName, From 67c9852e5cfb90cb014986c8c6d5e870f15493e3 Mon Sep 17 00:00:00 2001 From: Tejas Kumar Date: Tue, 18 May 2021 17:14:41 +0200 Subject: [PATCH 07/14] Remove period Signed-off-by: Tejas Kumar --- .../scaffolder-backend/src/scaffolder/stages/publish/helpers.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/stages/publish/helpers.ts b/plugins/scaffolder-backend/src/scaffolder/stages/publish/helpers.ts index 9d5effa8b2..fce3ca67da 100644 --- a/plugins/scaffolder-backend/src/scaffolder/stages/publish/helpers.ts +++ b/plugins/scaffolder-backend/src/scaffolder/stages/publish/helpers.ts @@ -93,7 +93,7 @@ export const enableBranchProtectionOnDefaultRepoBranch = async ({ /** * 👇 we need this preview because allowing a custom * reviewer count on branch protection is a preview - * feature. + * feature * * More here: https://docs.github.com/en/rest/overview/api-previews#require-multiple-approving-reviews */ From ce0872c4551a2202e50612c395b1d738bd06c82e Mon Sep 17 00:00:00 2001 From: Tejas Kumar Date: Tue, 18 May 2021 17:30:46 +0200 Subject: [PATCH 08/14] Add branch protection to newer publisher Signed-off-by: Tejas Kumar --- .../actions/builtin/publish/github.ts | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/github.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/github.ts index 66dfc1d655..1c4f56380b 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/github.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/github.ts @@ -19,7 +19,10 @@ import { ScmIntegrationRegistry, } from '@backstage/integration'; import { Octokit } from '@octokit/rest'; -import { initRepoAndPush } from '../../../stages/publish/helpers'; +import { + enableBranchProtectionOnDefaultRepoBranch, + initRepoAndPush, +} from '../../../stages/publish/helpers'; import { getRepoSourceDirectory, parseRepoUrl } from './util'; import { createTemplateAction } from '../../createTemplateAction'; @@ -171,7 +174,7 @@ export function createPublishGithubAction(options: { description: description, }); - const { data } = await repoCreationPromise; + const { data: newRepo } = await repoCreationPromise; if (access?.startsWith(`${owner}/`)) { const [, team] = access.split('/'); await client.teams.addOrUpdateRepoPermissionsInOrg({ @@ -212,8 +215,8 @@ export function createPublishGithubAction(options: { } } - const remoteUrl = data.clone_url; - const repoContentsUrl = `${data.html_url}/blob/master`; + const remoteUrl = newRepo.clone_url; + const repoContentsUrl = `${newRepo.html_url}/blob/master`; await initRepoAndPush({ dir: getRepoSourceDirectory(ctx.workspacePath, ctx.input.sourcePath), @@ -225,6 +228,16 @@ export function createPublishGithubAction(options: { logger: ctx.logger, }); + try { + await enableBranchProtectionOnDefaultRepoBranch({ + owner, + client, + repoName: newRepo.name, + }); + } catch (e) { + throw new Error(`Failed to add branch protection to '${name}', ${e}`); + } + ctx.output('remoteUrl', remoteUrl); ctx.output('repoContentsUrl', repoContentsUrl); }, From 0907e3c54f281622346d56c170e52c986dd54b52 Mon Sep 17 00:00:00 2001 From: Tejas Kumar Date: Tue, 18 May 2021 17:30:56 +0200 Subject: [PATCH 09/14] Add deprecation notice to deprecated publisher Signed-off-by: Tejas Kumar --- .../scaffolder-backend/src/scaffolder/stages/publish/github.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/plugins/scaffolder-backend/src/scaffolder/stages/publish/github.ts b/plugins/scaffolder-backend/src/scaffolder/stages/publish/github.ts index c7b7076095..1bb04904f0 100644 --- a/plugins/scaffolder-backend/src/scaffolder/stages/publish/github.ts +++ b/plugins/scaffolder-backend/src/scaffolder/stages/publish/github.ts @@ -29,6 +29,7 @@ import path from 'path'; export type RepoVisibilityOptions = 'private' | 'internal' | 'public'; +/** @deprecated */ export class GithubPublisher implements PublisherBase { static async fromConfig( config: GitHubIntegrationConfig, From e60676126079c5acd63e3f5cdda5b2d96c92608d Mon Sep 17 00:00:00 2001 From: Tejas Kumar Date: Tue, 18 May 2021 17:33:21 +0200 Subject: [PATCH 10/14] Clarify deprecation notice Signed-off-by: Tejas Kumar --- .../scaffolder-backend/src/scaffolder/stages/publish/github.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/stages/publish/github.ts b/plugins/scaffolder-backend/src/scaffolder/stages/publish/github.ts index 1bb04904f0..2647284698 100644 --- a/plugins/scaffolder-backend/src/scaffolder/stages/publish/github.ts +++ b/plugins/scaffolder-backend/src/scaffolder/stages/publish/github.ts @@ -29,7 +29,7 @@ import path from 'path'; export type RepoVisibilityOptions = 'private' | 'internal' | 'public'; -/** @deprecated */ +/** @deprecated use createPublishGithubAction instead */ export class GithubPublisher implements PublisherBase { static async fromConfig( config: GitHubIntegrationConfig, From 80a2bdc0be043a5c9578b3348aef4773a8509cbd Mon Sep 17 00:00:00 2001 From: Tejas Kumar Date: Wed, 19 May 2021 07:31:52 +0200 Subject: [PATCH 11/14] Use a literal for branch name Signed-off-by: Tejas Kumar --- .../src/scaffolder/stages/publish/helpers.ts | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/stages/publish/helpers.ts b/plugins/scaffolder-backend/src/scaffolder/stages/publish/helpers.ts index fce3ca67da..f1ad9617ce 100644 --- a/plugins/scaffolder-backend/src/scaffolder/stages/publish/helpers.ts +++ b/plugins/scaffolder-backend/src/scaffolder/stages/publish/helpers.ts @@ -82,11 +82,6 @@ export const enableBranchProtectionOnDefaultRepoBranch = async ({ owner, isRetry = false, }: BranchProtectionOptions): Promise => { - const { data: repo } = await client.repos.get({ - owner, - repo: repoName, - }); - try { await client.repos.updateBranchProtection({ mediaType: { @@ -101,7 +96,7 @@ export const enableBranchProtectionOnDefaultRepoBranch = async ({ }, owner, repo: repoName, - branch: repo.default_branch, + branch: 'master', required_status_checks: { strict: true, contexts: [] }, restrictions: null, enforce_admins: true, From d722343942c1755ee42f2a5df15dd02d6e5cfcfd Mon Sep 17 00:00:00 2001 From: Tejas Kumar Date: Wed, 19 May 2021 10:41:08 +0200 Subject: [PATCH 12/14] Oops. Adjust reference to repo `name`. Signed-off-by: Tejas Kumar --- .../src/scaffolder/actions/builtin/publish/github.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/github.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/github.ts index 1c4f56380b..e56c94a5b0 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/github.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/publish/github.ts @@ -235,7 +235,9 @@ export function createPublishGithubAction(options: { repoName: newRepo.name, }); } catch (e) { - throw new Error(`Failed to add branch protection to '${name}', ${e}`); + throw new Error( + `Failed to add branch protection to '${newRepo.name}', ${e}`, + ); } ctx.output('remoteUrl', remoteUrl); From 6c180284587f881fb5d2fe278997cfb4f6d13457 Mon Sep 17 00:00:00 2001 From: Tejas Kumar Date: Wed, 19 May 2021 10:46:55 +0200 Subject: [PATCH 13/14] Refactor to remove recursion Signed-off-by: Tejas Kumar --- .../src/scaffolder/stages/publish/helpers.ts | 29 ++++++++----------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/stages/publish/helpers.ts b/plugins/scaffolder-backend/src/scaffolder/stages/publish/helpers.ts index f1ad9617ce..6874efe06a 100644 --- a/plugins/scaffolder-backend/src/scaffolder/stages/publish/helpers.ts +++ b/plugins/scaffolder-backend/src/scaffolder/stages/publish/helpers.ts @@ -80,10 +80,9 @@ export const enableBranchProtectionOnDefaultRepoBranch = async ({ repoName, client, owner, - isRetry = false, }: BranchProtectionOptions): Promise => { - try { - await client.repos.updateBranchProtection({ + const tryOnce = () => { + return client.repos.updateBranchProtection({ mediaType: { /** * 👇 we need this preview because allowing a custom @@ -102,21 +101,17 @@ export const enableBranchProtectionOnDefaultRepoBranch = async ({ enforce_admins: true, required_pull_request_reviews: { required_approving_review_count: 1 }, }); + }; + + try { + await tryOnce(); } catch (e) { - if (!isRetry && e.message.includes('Branch not found')) { - // GitHub has eventual consistency. Fail silently, wait, and try again. - await new Promise(resolve => setTimeout(resolve, 600)); - - await enableBranchProtectionOnDefaultRepoBranch({ - repoName, - client, - owner, - isRetry: true, - }); - - return; + if (!e.message.includes('Branch not found')) { + throw e; } - - throw e; } + + // GitHub has eventual consistency. Fail silently, wait, and try again. + await new Promise(resolve => setTimeout(resolve, 600)); + await tryOnce(); }; From 903f3317edf64cb04ebc20a73a591e20211c6865 Mon Sep 17 00:00:00 2001 From: Tejas Kumar Date: Wed, 19 May 2021 10:59:41 +0200 Subject: [PATCH 14/14] Retry only on fail lol Signed-off-by: Tejas Kumar --- .../src/scaffolder/stages/publish/helpers.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/stages/publish/helpers.ts b/plugins/scaffolder-backend/src/scaffolder/stages/publish/helpers.ts index 6874efe06a..79c93128c6 100644 --- a/plugins/scaffolder-backend/src/scaffolder/stages/publish/helpers.ts +++ b/plugins/scaffolder-backend/src/scaffolder/stages/publish/helpers.ts @@ -109,9 +109,9 @@ export const enableBranchProtectionOnDefaultRepoBranch = async ({ if (!e.message.includes('Branch not found')) { throw e; } - } - // GitHub has eventual consistency. Fail silently, wait, and try again. - await new Promise(resolve => setTimeout(resolve, 600)); - await tryOnce(); + // GitHub has eventual consistency. Fail silently, wait, and try again. + await new Promise(resolve => setTimeout(resolve, 600)); + await tryOnce(); + } };