Abstracted code logic

[Bullrich]

Abstracted GitHub api interaction from the runner method.

This is the first step into isolating the code so it is easier to read.

Second step will isolate the logic

[rzadp]

I like it 👍 Feels good to abstract that logic.

[rzadp]

Suggested change

	const api = new GitHubApi(basePR, { logger, finishProcessReviews: null, octokit });
	const api = new GitHubApi(basePR, { logger, octokit });

It's not taken by GitHubApi. Same in other places.

[Bullrich]

I'm afraid it's a requirement in the object union that the method is receiving:

Argument of type '{ pr: PR; octokit: Octokit & { paginate: PaginateInterface; } & RestEndpointMethods & Api; logger: TestLogger; }' is not assignable to parameter of type 'Context & { pr: PR; }'.
Property 'finishProcessReviews' is missing in type '{ pr: PR; octokit: Octokit & { paginate: PaginateInterface; } & RestEndpointMethods & Api; logger: TestLogger; }' but required in type 'Context'.ts(2345)

I'll look into cleaning it in the future

[mutantcornholio]

I feel like this duplicates the purpose of opstooling-integrations. Should we consider reusing it instead?

[Bullrich]

opstooling-integrations

I would be up to use it but would need to check if I can adapt the specific endpoints and combinations that are being called.

Some stuff, like the following, may not be that straightforward to migrate.

octokit.paginate(
      this.octokit.rest.teams.listMembersInOrg,
      { org:pr.base.repo.owner.login, team_slug, per_page: maxGithubApiTeamMembersPerPage },
      (response) => response.data.map(({ login }) => login),
    );

And I would still keep it in a wrapper so it can be tested easier.

But this PR would be a step forward into that direction

[mutantcornholio]

Some stuff, like the following, may not be that straightforward to migrate.

octokit.paginate(
      this.octokit.rest.teams.listMembersInOrg,
      { org:pr.base.repo.owner.login, team_slug, per_page: maxGithubApiTeamMembersPerPage },
      (response) => response.data.map(({ login }) => login),
    );

We have paginated methods there :)

And I would still keep it in a wrapper so it can be tested easier.

One of the major points of creating opstooling-integrations was this exact goal: providing mocks for the endpoints from ground up, ease of use with jest.mock.

But this PR would be a step forward into that direction

Cool