[No QA] fixed quickly Commenting on PRs hit api limit

[parasharrajat]

Please review @roryabraham

Details

Added following changes to markPullRequestsAsDeployed GH action:

1. Send comment requests sequentially.
2. Added waiting period of one second after each comment request.

Fixed Issues

Fixes #2759

[parasharrajat]

I am sure it works as I have checked it with dummy promises but How should I test it in GH action context?

[roryabraham]

We unit test what we can, and sometimes test in other test repos, but unfortunately with our GH Actions we also just do a lot of live-testing by just merging PRs and seeing if it works 😅

[roryabraham]

So @parasharrajat I'm going to approve this but we need to hold on the merge for a bit because something is seriously wrong with our deploy checklist at the moment and I'm pretty sure it would affect our ability to test this PR

[roryabraham]

Also, keep in mind we won't really know if this works until we run a production deploy, which has been happening on average every two weeks for a while now, but could theoretically happen every day if there are no bugs on staging but not prod.

[parasharrajat]

I will wait. Just ping on the issue if it fails which is very unlikely

[parasharrajat]

Updated. I added Throttling based on https://github.com/actions/toolkit/tree/cac7db2d19f7a7e48fe1ec608fc99bc1786a858e/packages/github#extending-the-octokit-instance and removed the manual 1-sec waiting. To be sure we have to wait until it runs. Just ping here if there is an issue.

[roryabraham]

Nice work, just some minor comments!

[roryabraham]

I see that it was there before, but the octokit parameter is unnecessary / unused on this function. GithubUtils takes an octokit instance in its constructor.

[parasharrajat]

oh Yeah. I didn't notice.

[roryabraham]

This looks like some pretty generic code to set up the throttling, so can we move it to the GithubUtils constructor ?

We could make throttled an optional parameter that sets up throttling in the constructor, or just do it by default. Thoughts? Is there any time we wouldn't want throttling when we hit a rate limit?

[parasharrajat]

Throttling is good for all cases. Should we retry after Abuse Limit? It will by default wait for the Abuse locking period.

[roryabraham]

I guess for now we can not retry after the Abuse Limit – using throttling everywhere we shouldn't reach that limit unless something weird is going on.

[parasharrajat]

Agree.

[parasharrajat]

@roryabraham GithubUtils expects the first parameter as octokit instance but when we apply throttling, we have to create a new Octokit instance with the throttle plugin applied.

const OctokitThrottled = GitHub.plugin(throttling);

const octokit = new OctokitThrottled(getOctokitOptions(token, {

Now how do you want me to handle this?

1. We Pass a second parameter {throttle: Boolean}. which ignores the first parameter's octokit instance and creates new with the above logic.
2. We check for throttle property in the first parameter and if this property is found we Assume that we have passed the options as the first parameter and we will create our own Instance and if this property does not exist we assume that the first param was octokit instance so do whatever we do now.

[roryabraham]

I guess what I would do is just get rid of the octokit parameter in the GithubUtils constructor and just set it up like you have here, directly in the constructor when someone creates a GithubUtils instance. Then in all the actions that call new GithubUtils() just get rid of the octokit parameter. As a bonus step you could even make GithubUtils a singleton, such that all the methods become static and internally it will initialize the octokit instance if it needs to.

However, the only thing I'm unsure of in this approach is that when you do this:

const core = require('@actions/core');
const github = require('@actions/github');
const octokit = github.getOctokit(core.getInput('GITHUB_TOKEN', {required: true}));

You get a pre-authenticated octokit client. I'm not sure if that happens the same using this:

const {GitHub, getOctokitOptions} = require('@actions/github/lib/utils');

because you're not providing that GITHUB_TOKEN which is provided from the GH workflow that's calling the action.

[parasharrajat]

We pass the token here const octokit = new OctokitThrottled(getOctokitOptions(token <----, { I checked github.getOctokit( source. It basically does the the same.

[roryabraham]

Oh, nice 👍

So yeah I'd recommend that you just instantiate the octokit instance in the GithubUtils constructor. It's up to you if you want to make the class a singleton

[parasharrajat]

@roryabraham Were you expecting something like these changes? Please review.

[parasharrajat]

It seems Token is not being passed. Could you please help?

[roryabraham]

@parasharrajat try replacing this.octokit in all the static methods with GithubUtils.octokit ?

[roryabraham]

Also, if you find it more complex, then also just feel free to move the octokit initialization to the regular-old constructor and leave the non-static functions as non-static.

[parasharrajat]

@roryabraham I don't think this is complex. Seems it's better.
That this is fine it is referencing to the constructor. But it seems that tests are failing as we are not passing the Github_token from env.
https://github.com/Expensify/Expensify.cash/blob/5e50f17c4599071de29f90f62ab7f9bbf55e1602/.github/workflows/test.yml#L27

If we update the test.yml file to include GITHUB_TOKEN: ${{ secrets.OS_BOTIFY_TOKEN }} in env, will it work.

previously test file was using @octokit/rest and it was not using GITHUB_TOKEN which is like a public user. But now we updated it to GithubUtils which does not take octokit instance and now it's all static so we have to pass a token as its internal Octokit expects it.

I have one thing to bypass this.

1. Keep const {Octokit} = require('@octokit/rest');
2. Pass the new Octokit() to

GithubUtils.octokitInternal = new Octokit();

This bypasses the Octokit default instance creation. Should I do this instead?

[roryabraham]

@parasharrajat Actually I think what we want to do is just mock core.getInput for the GithubUtilsTest unit test. It's a unit test so we don't want or need a real token, because all the octokit functions are mocked too. We just need to mock core.getInput so that the constructor doesn't throw an error and I think these tests will pass again.

[parasharrajat]

Sorry, @roryabraham. But I do not seem to understand this. As per me we are using the real Octokit instance.

const {Octokit} = require('@octokit/rest');
const GithubUtils = require('../../.github/libs/GithubUtils');

on the top of the file.

It's a unit test so we don't want or need a real token

Agree to this, I think we can pass the Octokit instance from const {Octokit} = require('@octokit/rest'); like we were doing previously. But this time we have another way to do this.

GithubUtils.octokitInternal = new Octokit();

After this, I am sure the test will pass.

We are only mocking the Octokit at one place.

https://github.com/Expensify/Expensify.cash/blob/45b99bbaf7bbc33037c0d8ad8b8bb7aff7055611/tests/unit/GithubUtilsTest.js#L411

I think I should try with these changes and see if tests are passing. Anyways, it's not going to affect any real functionality.

[roryabraham]

@parasharrajat Okay, I'm not sure I'm following your solution, so feel free to update the PR so I can see what you mean. There are many examples of mocked octokit methods throughout the GithubUtilsTest.js file. Here's one.

So the solution I'm suggesting is that you don't change the implementation of static get octokit at all. What about something like this in the unit tests:

// This is a mock Octokit object. When we need to mock something in Octokit, we would just do something like:
// moctokit.issues.listForRepo = jest.fn().mockResolvedValue({data: []});
const moctokit = {};

beforeAll(() => {
    GithubUtils.octokitInternal = moctokit;
});

And then change all the instances like this one to:

// Before
GithubUtils.octokit.issues.listForRepo = jest.fn().mockResolvedValue({data: []});

// After
moctokit.issues.listForRepo = jest.fn().mockResolvedValue({data: []});

[parasharrajat]

@roryabraham Updated.

[roryabraham]

Okay, this is looking good.

[OSBotify]

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

[roryabraham]

Great news! It seems this didn't catastrophically break all our GH Actions, which means that it probably works as expected! 🙃