Skip to content

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

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

Merged
merged 9 commits into from
Apr 1, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
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
1 change: 1 addition & 0 deletions scripts/release/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,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 Down
56 changes: 40 additions & 16 deletions scripts/release/process-release.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import fsp from 'fs/promises';
import path from 'path';

import { Octokit } from '@octokit/rest';
import dotenv from 'dotenv';
import execa from 'execa';
import { copy, remove } from 'fs-extra';
Expand All @@ -23,6 +24,7 @@ import {
RELEASED_TAG,
OWNER,
REPO,
TEAM_SLUG,
getMarkdownSection,
configureGitHubAuthor,
cloneRepository,
Expand All @@ -32,6 +34,10 @@ import type { VersionsToRelease } from './types';

dotenv.config({ path: ROOT_ENV_PATH });

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

Choose a reason for hiding this comment

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

This could be in some common file

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'm not convinced on this. If we were to abstract all the octokit-related functions, then it'd make sense. But having only this part abstracted in a common file, I don't see much value. Besides, we have only two occurrences of new Octokit() across the code base. We could do it when there's a third time ;)

https://en.wikipedia.org/wiki/Rule_of_three_(computer_programming)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I count 3 ahah

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahhhh you win. I somehow saw only two.

Copy link
Collaborator

Choose a reason for hiding this comment

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

💪

Copy link
Contributor Author

Choose a reason for hiding this comment

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


type BeforeClientGenerationCommand = (params: {
releaseType: ReleaseType;
dir: string;
Expand All @@ -45,14 +51,16 @@ const BEFORE_CLIENT_GENERATION: {
},
};

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 {
data: { body },
} = await octokit.rest.issues.get({
owner: OWNER,
repo: REPO,
issue_number: Number(process.env.EVENT_NUMBER),
});

return body ?? '';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we fail if we can't find the body ? This seems critical

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 don't know why octokit provides such type definition, but according to their rest api docs, it doesn't mention anything about body being optional in the response. Even if it's not there, an empty string won't be a problem, because we need to parse the text and then proceed any release. And the empty string will prevent any further action.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes but it's better to bail early rather that doing a bunch of work that could fail later and don't know where the error is coming from, unless it's all silent in the end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it makes sense 097026e

}

function getDateStamp(): string {
Expand Down Expand Up @@ -149,6 +157,25 @@ async function updateChangelog({
);
}

async function isAuthorizedRelease(): Promise<boolean> {
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().includes('approved') &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe comment.body?.toLowerCase().trim() === 'approved would be safer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it should be more stricter. 1c66be4

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 @@ -158,16 +185,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
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'),
};