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

eunjae-lee · 2022-03-31T09:26:20Z

🧭 What and Why

🎟 JIRA Ticket: https://algolia.atlassian.net/browse/APIC-315

Changes included:

Creates a release issue without checkbox, but an explanation on approval comment.
Processes release only if there is an approval comment in the release issue.

netlify · 2022-03-31T09:26:26Z

✅ Deploy Preview for api-clients-automation canceled.

Name	Link
🔨 Latest commit	`4a8971e`
🔍 Latest deploy log	https://app.netlify.com/sites/api-clients-automation/deploys/6245c20f009e840009c244d1

algolia-bot · 2022-03-31T09:27:29Z

✗ The generated branch has been deleted.

If the PR has been merged, you can check the generated code on the generated/main branch.

scripts/release/process-release.ts

shortcuts · 2022-03-31T10:16:08Z

scripts/release/process-release.ts

+
+  const { data: members } = await octokit.rest.teams.listMembersInOrg({
+    org: OWNER,
+    team_slug: TEAM_SLUG,


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?

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?

Maybe anyone at algolia should be able to release ?

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

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.

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

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

millotp · 2022-03-31T13:46:47Z

scripts/release/process-release.ts

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


This could be in some common file

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)

I count 3 ahah

ahhhh you win. I somehow saw only two.

millotp · 2022-03-31T13:49:19Z

scripts/release/process-release.ts

+    issue_number: Number(process.env.EVENT_NUMBER),
+  });
+
+  return body ?? '';


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

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.

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

Yeah it makes sense 097026e

millotp · 2022-03-31T13:50:53Z

scripts/release/process-release.ts

+
+  return comments.some(
+    (comment) =>
+      comment.body?.toLowerCase().includes('approved') &&


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

Yeah it should be more stricter. 1c66be4

millotp · 2022-03-31T14:39:18Z

scripts/release/common.ts

@@ -18,6 +21,12 @@ export const MAIN_PACKAGE = Object.keys(clientsConfig).reduce(
  {}
 );

+export function getOctokit(githubToken: string): Octokit {


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

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.

shortcuts

Small comment but looks good otherwise!

shortcuts · 2022-03-31T14:41:32Z

scripts/release/common.ts

@@ -18,6 +21,12 @@ export const MAIN_PACKAGE = Object.keys(clientsConfig).reduce(
  {}
 );

+export function getOctokit(githubToken: string): Octokit {


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

ah bah what Pierre said

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.

shortcuts · 2022-03-31T14:42:23Z

scripts/release/process-release.ts

+
+  const { data: members } = await octokit.rest.teams.listMembersInOrg({
+    org: OWNER,
+    team_slug: TEAM_SLUG,


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

millotp

Let's go !

* chore(ci): authenticate release issue by approval comment * chore: fix lint error * chore: clean up * chore: validate comment body strictly * chore: early return if body does not exist * chore: extract octokit generation * chore: remove unused import

chore(ci): authenticate release issue by approval comment

fb6392c

chore: fix lint error

21ce8c0

eunjae-lee requested review from a team, damcou and shortcuts and removed request for a team March 31, 2022 09:43

shortcuts reviewed Mar 31, 2022

View reviewed changes

eunjae-lee added 2 commits March 31, 2022 15:11

chore: clean up

fd73907

Merge branch 'main' into chore/authentication

436b77e

millotp reviewed Mar 31, 2022

View reviewed changes

eunjae-lee added 4 commits March 31, 2022 16:02

chore: validate comment body strictly

1c66be4

chore: early return if body does not exist

097026e

chore: extract octokit generation

3239ac8

chore: remove unused import

6794008

millotp previously approved these changes Mar 31, 2022

View reviewed changes

shortcuts reviewed Mar 31, 2022

View reviewed changes

Merge branch 'main' into chore/authentication

4a8971e

eunjae-lee dismissed millotp’s stale review via 4a8971e March 31, 2022 15:00

millotp approved these changes Apr 1, 2022

View reviewed changes

eunjae-lee merged commit 832d12f into main Apr 1, 2022

eunjae-lee deleted the chore/authentication branch April 1, 2022 08:52

shortcuts mentioned this pull request May 25, 2022

chore: prepare release 2022-05-25 #549

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

eunjae-lee commented Mar 31, 2022

netlify bot commented Mar 31, 2022 •

edited

Loading

algolia-bot commented Mar 31, 2022 •

edited

Loading

shortcuts Mar 31, 2022

eunjae-lee Mar 31, 2022

millotp Mar 31, 2022

shortcuts Mar 31, 2022

eunjae-lee Mar 31, 2022

bodinsamuel Mar 31, 2022

shortcuts Mar 31, 2022

millotp Mar 31, 2022

eunjae-lee Mar 31, 2022

millotp Mar 31, 2022

eunjae-lee Mar 31, 2022

millotp Mar 31, 2022

eunjae-lee Mar 31, 2022

millotp Mar 31, 2022

eunjae-lee Mar 31, 2022

millotp Mar 31, 2022

eunjae-lee Mar 31, 2022

millotp Mar 31, 2022

eunjae-lee Mar 31, 2022

millotp Mar 31, 2022

eunjae-lee Mar 31, 2022

shortcuts left a comment

shortcuts Mar 31, 2022

shortcuts Mar 31, 2022

eunjae-lee Mar 31, 2022

shortcuts Mar 31, 2022

millotp left a comment

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

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

Conversation

eunjae-lee commented Mar 31, 2022

🧭 What and Why

Changes included:

netlify bot commented Mar 31, 2022 • edited Loading

✅ Deploy Preview for api-clients-automation canceled.

algolia-bot commented Mar 31, 2022 • edited Loading

✗ The generated branch has been deleted.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shortcuts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

millotp left a comment

Choose a reason for hiding this comment

netlify bot commented Mar 31, 2022 •

edited

Loading

algolia-bot commented Mar 31, 2022 •

edited

Loading