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.

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

Fellow members fetching #79

Merged
merged 18 commits into from
Sep 15, 2023
Merged
Show file tree
Hide file tree
Changes from 16 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
19 changes: 14 additions & 5 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,7 @@ rules:
users:
- user-1
- user-2
rank: 2
Bullrich marked this conversation as resolved.
Show resolved Hide resolved
countAuthor: true
allowedToSkipRule:
teams:
Expand All @@ -256,9 +257,13 @@ It has the same parameters as a normal rule:
- **Optional**: Defaults to 1.
- Must be greater than the number of users available (you cannot request 5 approvals from a team of 4 users)
- **teams**: An array of team *slugs* that need to review this file.
- *Optional if **users** is defined*.
- *Optional if **rank** or **users** are defined*.
- **users**: An array of the GitHub usernames of the users who need to review this file.
- *Optional if **teams** is defined*.
- *Optional if **teams** or **rank** are defined*.
- **rank**: A number defining the minimum [rank a fellow](https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Fkusama.api.onfinality.io%2Fpublic-ws#/fellowship) needs to have to approve this PR.
- *Optional if **teams** or **users** are defined*.
- If it is set to rank 3, it will need the approval of a fellow of rank 3 or higher to pass.
- The username is fetched [from the additional field](https://github.com/polkadot-fellows/runtimes/issues/7) in the identity of the fellows. If it is null, the fellow’s review won’t count towards the required approval.
- **countAuthor**: If the pull request author should be considered as an approval.
- If the author belongs to the list of approved users (either by team or by users) his approval will be counted (requiring one less approvals in total).
- ** Optional**: Defaults to `false`
Expand Down Expand Up @@ -303,9 +308,13 @@ rules:
- **Optional**: Defaults to 1.
- Must be greater than the number of users available (you cannot request 5 approvals from a team of 4 users)
- **teams**: An array of team *slugs* that need to review this file.
- *Optional if **users** is defined*.
- *Optional if **rank** or **users** are defined*.
- **users**: An array of the GitHub usernames of the users who need to review this file.
- *Optional if **teams** is defined*.
- *Optional if **teams** or **rank** are defined*.
- **rank**: A number defining the minimum [rank a fellow](https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Fkusama.api.onfinality.io%2Fpublic-ws#/fellowship) needs to have to approve this PR.
- *Optional if **teams** or **users** are defined*.
- If it is set to rank 3, it will need the approval of a fellow of rank 3 or higher to pass.
- The username is fetched [from the additional field](https://github.com/polkadot-fellows/runtimes/issues/7) in the identity of the fellows. If it is null, the fellow’s review won’t count towards the required approval.
##### Or rule logic
This is a rule that has at least two available options of reviewers and needs **at least one group to approve**.

Expand Down Expand Up @@ -381,4 +390,4 @@ To deploy a new version you need to update two files:

When a commit is pushed to the main branch and the versions have changed, the system will automatically tag the commit and release a new package with such version.

You can find all the available versions in the [release section](./releases).
You can find all the available versions in the [release section](../releases).
7 changes: 6 additions & 1 deletion jest.config.js
Original file line number Diff line number Diff line change
@@ -1,2 +1,7 @@
/** @type {import('ts-jest').JestConfigWithTsJest} */
module.exports = { preset: "ts-jest", testEnvironment: "node", testMatch: [__dirname + "/src/**/test/**/*.test.ts"] };
module.exports = {
preset: "ts-jest",
testEnvironment: "node",
testTimeout: 8_000,
testMatch: [__dirname + "/src/**/test/**/*.test.ts"],
};
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
"@actions/core": "^1.10.0",
"@actions/github": "^5.1.1",
"@eng-automation/js": "^1.0.2",
"@polkadot/api": "^10.9.1",
"joi": "^17.10.0",
"yaml": "^2.3.2"
}
Expand Down
11 changes: 1 addition & 10 deletions src/github/teams.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,4 @@
import { ActionLogger, GitHubClient } from "./types";

/**
* Interface for the acquisition of members of a team.
* As we may be using blockchain instead of GitHub teams, let's wrap the functionality inside a interface
*/
export interface TeamApi {
/** Returns all the GitHub account's logins which belong to a given team. */
getTeamMembers(teamName: string): Promise<string[]>;
}
import { ActionLogger, GitHubClient, TeamApi } from "./types";

/**
* Implementation of the TeamApi interface using GitHub teams
Expand Down
9 changes: 9 additions & 0 deletions src/github/types.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,14 @@
import type { GitHub } from "@actions/github/lib/utils";

/**
* Interface for the acquisition of members of a team.
* As we may be using blockchain instead of GitHub teams, let's wrap the functionality inside a interface
*/
export interface TeamApi {
/** Returns all the GitHub account's logins which belong to a given team. */
getTeamMembers(teamName: string): Promise<string[]>;
}

export interface ActionLogger {
debug(message: string): void;
info(message: string): void;
Expand Down
5 changes: 4 additions & 1 deletion src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { PullRequest } from "@octokit/webhooks-types";
import { GitHubChecksApi } from "./github/check";
import { PullRequestApi } from "./github/pullRequest";
import { GitHubTeamsApi } from "./github/teams";
import { PolkadotFellows } from "./polkadot/fellows";
import { ActionRunner } from "./runner";
import { generateCoreLogger } from "./util";

Expand Down Expand Up @@ -63,7 +64,9 @@ const teamApi = new GitHubTeamsApi(getOctokit(inputs.teamApiToken), repo.owner,

const checks = new GitHubChecksApi(getOctokit(inputs.teamApiToken), pr, logger, actionId);

const runner = new ActionRunner(api, teamApi, checks, logger);
const fellows = new PolkadotFellows(logger);

const runner = new ActionRunner(api, teamApi, fellows, checks, logger);

runner
.runAction(inputs)
Expand Down
108 changes: 108 additions & 0 deletions src/polkadot/fellows.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
/* eslint-disable @typescript-eslint/ban-ts-comment */
import { ApiPromise, WsProvider } from "@polkadot/api";

import { ActionLogger, TeamApi } from "../github/types";

type FellowData = { address: string; rank: number };

export class PolkadotFellows implements TeamApi {
private fellowsCache: Map<string, number> = new Map<string, number>();

constructor(private readonly logger: ActionLogger) {}

async fetchAllFellows(): Promise<Map<string, number>> {
let api: ApiPromise;
this.logger.debug("Connecting to collective parachain");
// we connect to the collective rpc node
const wsProvider = new WsProvider("wss://polkadot-collectives-rpc.polkadot.io");
api = await ApiPromise.create({ provider: wsProvider });
try {
// We fetch all the members
const membersObj = await api.query.fellowshipCollective.members.entries();

// We iterate over the fellow data and convert them into usable values
const fellows: FellowData[] = [];
for (const [key, rank] of membersObj) {
// @ts-ignore
const [address]: [string] = key.toHuman();
fellows.push({ address, ...(rank.toHuman() as object) } as FellowData);
Comment on lines +27 to +28
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, is it basically this?

fellows.push({address: key.toHuman()[0], rank: rank.toHuman()[0]});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha yes.

I just used destructuring so that I doesn't look like I'm accessing random array elements which makes it a bit harder to read, but yes.

key is a string with the address, and rank is {rank: 1}.

}
this.logger.debug(JSON.stringify(fellows));

// Once we obtained this information, we disconnect this api.
await api.disconnect();

this.logger.debug("Connecting to relay parachain.");
// We connect to the relay chain
api = await ApiPromise.create({ provider: new WsProvider("wss://rpc.polkadot.io") });

// We iterate over the different members and extract their data
const users: Map<string, number> = new Map<string, number>();
for (const fellow of fellows) {
this.logger.debug(`Fetching identity of '${fellow.address}', rank: ${fellow.rank}`);
const fellowData = (await api.query.identity.identityOf(fellow.address)).toHuman() as
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this deserves a semaphore, so it could be queried in parallel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you know if there is a limit to stop abuse? That was my main concern.

The current execution takes about 2s, which given the fact that this is an action, is not really a problem (the whole action is executed in about 5s).

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know if there is a limit to stop abuse? That was my main concern.

Could you elaborate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same problem that we have with GitHub, which has a rate limit.

I assume that there is some kind of rate limit when using Polkadot-JS (else what stop people from DOSing it?), and, because I'm fetching 60 accounts, I found it better to do it in order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I'm not understanding you. Could you please elaborate on what you had in mind?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we're speaking of the same thing here. I suggest adding a semaphore, so requests would be processed in parallel, but with a limit on how much parallel requests to make.

In gitspiegel, I've used semaphore-promise library for that.
Check out the usage in core.ts

| Record<string, unknown>
| undefined;

// If the identity is null, we ignore it.
if (!fellowData) {
this.logger.debug("Identity is null. Skipping");
continue;
}

// @ts-ignore
const additional = fellowData.info.additional as [{ Raw: string }, { Raw: string }][] | undefined;

// If it does not have additional data (GitHub handle goes here) we ignore it
if (!additional || additional.length < 1) {
this.logger.debug("Additional data is null. Skipping");
continue;
}

for (const additionalData of additional) {
const [key, value] = additionalData;
// We verify that they have an additional data of the key "github"
// If it has a handle defined, we push it into the array
if (key?.Raw && key?.Raw === "github" && value?.Raw && value?.Raw.length > 0) {
this.logger.debug(`Found handles: '${value.Raw}'`);
// We add it to the array and remove the @ if they add it to the handle
users.set(value.Raw.replace("@", ""), fellow.rank);
}
}
}

this.logger.info(`Found users: ${JSON.stringify(Array.from(users.entries()))}`);

return users;
} catch (error) {
this.logger.error(error as Error);
throw error;
} finally {
await api.disconnect();
}
}

async getTeamMembers(ranking: string): Promise<string[]> {
const requiredRank = Number(ranking);
this.logger.info(`Fetching members of rank '${requiredRank}' or higher`);

if (this.fellowsCache.size < 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this will use data from cache in case if there are more than 1 rule bucket with rank: x?
i don't see any other use case then... right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a map. The idea for this is to fetch all the available fellows once and then we filter per request, instead of fetching them multiple times per required rank.

So if you request in one rule a rank 4, it will already fetch them all, so, if you request later rank 2, all the ranks will be available already.

this.logger.debug("Cache not found. Fetching fellows.");
this.fellowsCache = await this.fetchAllFellows();
}
const users: string[] = [];
for (const [user, rank] of this.fellowsCache) {
if (rank >= requiredRank) {
users.push(user);
}
}

if (users.length === 0) {
throw new Error(`Found no members of rank ${requiredRank} or higher. Please see debug logs`);
}

this.logger.info(`GitHub members of rank '${requiredRank}' or higher are: ${users.join(",")}`);

return users;
}
}
2 changes: 1 addition & 1 deletion src/rules/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ export enum RuleTypes {
AndDistinct = "and-distinct",
}

export type Reviewers = { users?: string[]; teams?: string[]; min_approvals: number };
export type Reviewers = { users?: string[]; teams?: string[]; rank?: number; min_approvals: number };

export interface Rule {
name: string;
Expand Down
7 changes: 4 additions & 3 deletions src/rules/validator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { AndRule, BasicRule, ConfigurationFile, Reviewers, Rule, RuleTypes } fro
const reviewersObj = {
users: Joi.array().items(Joi.string()).optional().empty(null),
teams: Joi.array().items(Joi.string()).optional().empty(null),
rank: Joi.number().optional().min(1).empty(null),
};

const reviewerConditionObj = { ...reviewersObj, min_approvals: Joi.number().min(1).default(1) };
Expand All @@ -34,20 +35,20 @@ const ruleSchema = Joi.object<Rule & { type: string }>().keys({
*/
export const generalSchema = Joi.object<ConfigurationFile>().keys({
rules: Joi.array<ConfigurationFile["rules"]>().items(ruleSchema).unique("name").required(),
preventReviewRequests: Joi.object().keys(reviewersObj).optional().or("users", "teams"),
preventReviewRequests: Joi.object().keys(reviewersObj).optional().or("users", "teams", "rank"),
});

/** Basic rule schema
* This rule is quite simple as it only has the min_approvals field and the required reviewers
*/
export const basicRuleSchema = Joi.object<BasicRule>()
.keys({ ...reviewerConditionObj, countAuthor: Joi.boolean().default(false) })
.or("users", "teams");
.or("users", "teams", "rank");

/** As, with the exception of basic, every other schema has the same structure, we can recycle this */
export const otherRulesSchema = Joi.object<AndRule>().keys({
reviewers: Joi.array<AndRule["reviewers"]>()
.items(Joi.object<Reviewers>().keys(reviewerConditionObj).or("users", "teams"))
.items(Joi.object<Reviewers>().keys(reviewerConditionObj).or("users", "teams", "rank"))
.min(2)
.required(),
countAuthor: Joi.boolean().default(false),
Expand Down
10 changes: 8 additions & 2 deletions src/runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,7 @@ import { parse } from "yaml";
import { Inputs } from ".";
import { GitHubChecksApi } from "./github/check";
import { PullRequestApi } from "./github/pullRequest";
import { TeamApi } from "./github/teams";
import { ActionLogger, CheckData } from "./github/types";
import { ActionLogger, CheckData, TeamApi } from "./github/types";
import { AndDistinctRule, ConfigurationFile, Reviewers, Rule } from "./rules/types";
import { validateConfig, validateRegularExpressions } from "./rules/validator";
import { caseInsensitiveEqual, concatArraysUniquely } from "./util";
Expand Down Expand Up @@ -37,6 +36,7 @@ export class ActionRunner {
constructor(
private readonly prApi: PullRequestApi,
private readonly teamApi: TeamApi,
private readonly polkadotApi: TeamApi,
private readonly checks: GitHubChecksApi,
private readonly logger: ActionLogger,
) {}
Expand Down Expand Up @@ -472,6 +472,12 @@ export class ActionRunner {
users.add(user);
}
}
if (reviewers.rank) {
const members = await this.polkadotApi.getTeamMembers(reviewers.rank.toString());
for (const member of members) {
users.add(member);
}
}

return Array.from(users);
}
Expand Down
30 changes: 30 additions & 0 deletions src/test/fellows.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/* eslint-disable @typescript-eslint/unbound-method */
import { mock, mockClear, MockProxy } from "jest-mock-extended";

import { ActionLogger, TeamApi } from "../github/types";
import { PolkadotFellows } from "../polkadot/fellows";

describe("CAPI test", () => {
let fellows: TeamApi;
let logger: MockProxy<ActionLogger>;

beforeEach(() => {
logger = mock<ActionLogger>();
fellows = new PolkadotFellows(logger);
});

test("Should fetch fellows", async () => {
const members = await fellows.getTeamMembers("2");
expect(members.length).toBeGreaterThan(0);
});

test("Should cache fellows", async () => {
const members = await fellows.getTeamMembers("2");
expect(members.length).toBeGreaterThan(0);
expect(logger.debug).toHaveBeenCalledWith("Connecting to collective parachain");
mockClear(logger);
const members2 = await fellows.getTeamMembers("2");
expect(members2.length).toBeGreaterThan(0);
expect(logger.debug).not.toHaveBeenCalledWith("Connecting to collective parachain");
Copy link
Contributor

@mordamax mordamax Sep 13, 2023

Choose a reason for hiding this comment

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

this is nice test. do you plan to add the actual rule with rank:2 where you see it allows / or disallows if approver is rank < 2 ?

UPD: ah I see you keep adding stuff :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good question. I didn't plan on adding it, but maybe it's no trouble at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It actually runs out of the box, so that's good!

Copy link
Contributor

@mordamax mordamax Sep 15, 2023

Choose a reason for hiding this comment

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

It actually runs out of the box, so that's good!

where? I haven't seen in this diff changes where the rank: 2 approvals are being tested, could you please point my eyes?

FYI: if there's no test, not a blocker for this PR, just needs to be done now or later

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 added a test to src/test/runner/runner.test.ts where it verifies that the user is of a given rank and skips the test.

Copy link
Contributor

@mordamax mordamax Sep 15, 2023

Choose a reason for hiding this comment

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

hmm.. so if author is fellow - it means he doesn't need review? or I don't get
I thought they need to check whether someone who has approved - is a fellow with dan >= X, so then it counts. otherwise the review-bot blocks merge. So I was expecting this kind of test

});
});
6 changes: 3 additions & 3 deletions src/test/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ import { join } from "path";

import { GitHubChecksApi } from "../github/check";
import { PullRequestApi } from "../github/pullRequest";
import { GitHubTeamsApi, TeamApi } from "../github/teams";
import { ActionLogger, GitHubClient } from "../github/types";
import { GitHubTeamsApi } from "../github/teams";
import { ActionLogger, GitHubClient, TeamApi } from "../github/types";
import { ActionRunner, RuleReport } from "../runner";

type ReportName =
Expand Down Expand Up @@ -83,7 +83,7 @@ describe("Integration testing", () => {
api = new PullRequestApi(client, pr, logger, "");
teams = new GitHubTeamsApi(client, "org", logger);
checks = new GitHubChecksApi(client, pr, logger, "example");
runner = new ActionRunner(api, teams, checks, logger);
runner = new ActionRunner(api, teams, mock<TeamApi>(), checks, logger);

// @ts-ignore problem with the type being mocked
client.rest.repos.getContent.mockResolvedValue({ data: { content: Buffer.from(config, "utf-8") } });
Expand Down
5 changes: 2 additions & 3 deletions src/test/rules/andDistinct.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@ import { mock, MockProxy } from "jest-mock-extended";

import { GitHubChecksApi } from "../../github/check";
import { PullRequestApi } from "../../github/pullRequest";
import { TeamApi } from "../../github/teams";
import { ActionLogger } from "../../github/types";
import { ActionLogger, TeamApi } from "../../github/types";
import { AndDistinctRule } from "../../rules/types";
import { ActionRunner } from "../../runner";

Expand All @@ -16,7 +15,7 @@ describe("'AndDistinctRule' rule parsing", () => {
let teamsApi: MockProxy<TeamApi>;
beforeEach(() => {
api = mock<PullRequestApi>();
runner = new ActionRunner(api, teamsApi, mock<GitHubChecksApi>(), mock<ActionLogger>());
runner = new ActionRunner(api, teamsApi, mock<TeamApi>(), mock<GitHubChecksApi>(), mock<ActionLogger>());
});
test("should get minimal config", async () => {
api.getConfigFile.mockResolvedValue(`
Expand Down
5 changes: 2 additions & 3 deletions src/test/rules/andRule.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@ import { mock, MockProxy } from "jest-mock-extended";

import { GitHubChecksApi } from "../../github/check";
import { PullRequestApi } from "../../github/pullRequest";
import { TeamApi } from "../../github/teams";
import { ActionLogger } from "../../github/types";
import { ActionLogger, TeamApi } from "../../github/types";
import { AndRule } from "../../rules/types";
import { ActionRunner } from "../../runner";

Expand All @@ -16,7 +15,7 @@ describe("'And' rule parsing", () => {
let teamsApi: MockProxy<TeamApi>;
beforeEach(() => {
api = mock<PullRequestApi>();
runner = new ActionRunner(api, teamsApi, mock<GitHubChecksApi>(), mock<ActionLogger>());
runner = new ActionRunner(api, teamsApi, mock<TeamApi>(), mock<GitHubChecksApi>(), mock<ActionLogger>());
});
test("should get minimal config", async () => {
api.getConfigFile.mockResolvedValue(`
Expand Down
Loading
Loading