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.

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

chore(ci): authenticate release issue by approval comment #316

Merged
merged 9 commits into from
Apr 1, 2022
Merged

chore(ci): authenticate release issue by approval comment #316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
fb6392c
chore(ci): authenticate release issue by approval comment
eunjae-lee Mar 31, 2022
21ce8c0
chore: fix lint error
eunjae-lee Mar 31, 2022
fd73907
chore: clean up
eunjae-lee Mar 31, 2022
436b77e
Merge branch 'main' into chore/authentication
eunjae-lee Mar 31, 2022
1c66be4
chore: validate comment body strictly
eunjae-lee Mar 31, 2022
097026e
chore: early return if body does not exist
eunjae-lee Mar 31, 2022
3239ac8
chore: extract octokit generation
eunjae-lee Mar 31, 2022
6794008
chore: remove unused import
eunjae-lee Mar 31, 2022
4a8971e
Merge branch 'main' into chore/authentication
eunjae-lee Mar 31, 2022
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
1 change: 1 addition & 0 deletions config/release.config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"mainBranch": "main",
"owner": "algolia",
"repo": "api-clients-automation",
"teamSlug": "api-clients-automation",
"targetBranch": {
"javascript": "next",
"php": "next",
Expand Down
8 changes: 2 additions & 6 deletions scripts/ci/codegen/upsertGenerationComment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,12 @@
/* eslint-disable no-console */
import { Octokit } from '@octokit/rest';

import { run } from '../../common';
import { OWNER, REPO } from '../../release/common';
import { getOctokit, OWNER, REPO } from '../../release/common';

import commentText from './text';

const BOT_NAME = 'algolia-bot';
const PR_NUMBER = parseInt(process.env.PR_NUMBER || '0', 10);
const octokit = new Octokit({
auth: `token ${process.env.GITHUB_TOKEN}`,
});
const octokit = getOctokit(process.env.GITHUB_TOKEN!);

const args = process.argv.slice(2);
const allowedTriggers = ['notification', 'codegen', 'noGen', 'cleanup'];
Expand Down
9 changes: 9 additions & 0 deletions scripts/release/common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import path from 'path';

import { Octokit } from '@octokit/rest';

import clientsConfig from '../../config/clients.config.json';
import config from '../../config/release.config.json';
import { getGitHubUrl, run } from '../common';
Expand All @@ -8,6 +10,7 @@ export const RELEASED_TAG = config.releasedTag;
export const MAIN_BRANCH = config.mainBranch;
export const OWNER = config.owner;
export const REPO = config.repo;
export const TEAM_SLUG = config.teamSlug;
export const MAIN_PACKAGE = Object.keys(clientsConfig).reduce(
(mainPackage: { [lang: string]: string }, lang: string) => {
return {
Expand All @@ -18,6 +21,12 @@ export const MAIN_PACKAGE = Object.keys(clientsConfig).reduce(
{}
);

export function getOctokit(githubToken: string): Octokit {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't need to build a new octokit everytime, this should be a local variable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What if we want to pass a different github token? I think it's easy to maintain stuff in common without directly accessing process.env.xxx.

Copy link
Member

Choose a reason for hiding this comment

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

Unless we have different GH tokens, we can have a variable for it

Copy link
Member

Choose a reason for hiding this comment

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

ah bah what Pierre said

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Environment variables travel through process-release.yml -> process-release.ts, and I don't think it's good to have code accessing environment variables directly inside common, because we don't know where the functions in common can be used, and we cannot ensure if the env var exists.

return new Octokit({
auth: `token ${githubToken}`,
});
}

export function getTargetBranch(language: string): string {
return config.targetBranch[language] || config.defaultTargetBranch;
}
Expand Down
14 changes: 9 additions & 5 deletions scripts/release/create-release-issue.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,18 @@
/* eslint-disable no-console */
import { Octokit } from '@octokit/rest';
import dotenv from 'dotenv';
import semver from 'semver';

import { LANGUAGES, ROOT_ENV_PATH, run, getPackageVersion } from '../common';
import type { Language } from '../types';

import { RELEASED_TAG, MAIN_BRANCH, OWNER, REPO, MAIN_PACKAGE } from './common';
import {
RELEASED_TAG,
MAIN_BRANCH,
OWNER,
REPO,
MAIN_PACKAGE,
getOctokit,
} from './common';
import TEXT from './text';
import type {
Versions,
Expand Down Expand Up @@ -246,9 +252,7 @@ async function createReleaseIssue(): Promise<void> {
TEXT.approval,
].join('\n\n');

const octokit = new Octokit({
auth: `token ${process.env.GITHUB_TOKEN}`,
});
const octokit = getOctokit(process.env.GITHUB_TOKEN!);

octokit.rest.issues
.create({
Expand Down
59 changes: 43 additions & 16 deletions scripts/release/process-release.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,11 @@ import {
RELEASED_TAG,
OWNER,
REPO,
TEAM_SLUG,
getMarkdownSection,
configureGitHubAuthor,
cloneRepository,
getOctokit,
} from './common';
import TEXT from './text';
import type {
Expand All @@ -50,14 +52,22 @@ const BEFORE_CLIENT_COMMIT: { [lang: string]: BeforeClientCommitCommand } = {
},
};

function getIssueBody(): string {
return JSON.parse(
execa.sync('curl', [
'-H',
`Authorization: token ${process.env.GITHUB_TOKEN}`,
`https://api.github.com/repos/${OWNER}/${REPO}/issues/${process.env.EVENT_NUMBER}`,
]).stdout
).body;
async function getIssueBody(): Promise<string> {
const octokit = getOctokit(process.env.GITHUB_TOKEN!);
const {
data: { body },
} = await octokit.rest.issues.get({
owner: OWNER,
repo: REPO,
issue_number: Number(process.env.EVENT_NUMBER),
});

if (!body) {
throw new Error(
`Unexpected \`body\` of the release issue: ${JSON.stringify(body)}`
);
}
return body;
}

function getDateStamp(): string {
Expand Down Expand Up @@ -154,6 +164,26 @@ async function updateChangelog({
);
}

async function isAuthorizedRelease(): Promise<boolean> {
const octokit = getOctokit(process.env.GITHUB_TOKEN!);
const { data: members } = await octokit.rest.teams.listMembersInOrg({
org: OWNER,
team_slug: TEAM_SLUG,
Copy link
Member

Choose a reason for hiding this comment

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

Although I get the idea, I wonder if it's good to check if it's a team member.

The end goal of this project is to let anyone from the company contribute to our clients, which would not match this condition.

Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's still okay. Anyone can open pull-requests and contribute to the code base, but only us can trigger releases. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe anyone at algolia should be able to release ?

Copy link
Member

Choose a reason for hiding this comment

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

We would still be the bottleneck, IMO anyone from the company should have the rights to do it, we are just here to make sure it works well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why don't we start with a small scope and expand later if there's a demand and if we think it makes sense to allow them to trigger releases? Anyway we can replace this small piece of code quite easily if we want.

Copy link
Contributor

Choose a reason for hiding this comment

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

yep I think it's fair at the moment to start with only us. We want anyone to contribute but it's very unlikely that we want unattended release

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I was mostly pointing it out but let's not forget about this

});

const { data: comments } = await octokit.rest.issues.listComments({
owner: OWNER,
repo: REPO,
issue_number: Number(process.env.EVENT_NUMBER),
});

return comments.some(
(comment) =>
comment.body?.toLowerCase().trim() === 'approved' &&
members.find((member) => member.login === comment.user?.login)
);
}

async function processRelease(): Promise<void> {
if (!process.env.GITHUB_TOKEN) {
throw new Error('Environment variable `GITHUB_TOKEN` does not exist.');
Expand All @@ -163,16 +193,13 @@ async function processRelease(): Promise<void> {
throw new Error('Environment variable `EVENT_NUMBER` does not exist.');
}

const issueBody = getIssueBody();

if (
!getMarkdownSection(issueBody, TEXT.approvalHeader)
.split('\n')
.find((line) => line.startsWith(`- [x] ${TEXT.approved}`))
) {
throw new Error('The issue was not approved.');
if (!(await isAuthorizedRelease())) {
throw new Error(
'The issue was not approved.\nA team member must leave a comment "approved" in the release issue.'
);
}

const issueBody = await getIssueBody();
const versionsToRelease = getVersionsToRelease(issueBody);

await updateOpenApiTools(versionsToRelease);
Expand Down
8 changes: 2 additions & 6 deletions scripts/release/text.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
const APPROVED = `Approved`;

export default {
header: `## Summary`,

Expand Down Expand Up @@ -33,10 +31,8 @@ export default {
changelogDescription: `Update the following lines. Once merged, it will be reflected to \`changelogs/*.\``,

approvalHeader: `## Approval`,
approved: APPROVED,
approval: [
`To proceed this release, check the box below and close the issue.`,
`To skip this release, just close the issue.`,
`- [ ] ${APPROVED}`,
`To proceed this release, a team member must leave a comment "approved" in this issue.`,
`To skip this release, just close it.`,
].join('\n'),
};