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

fix: retry requests on 401 response within first minute #85

Merged
merged 6 commits into from Jun 3, 2020

Conversation

gr2m
Copy link
Contributor

@gr2m gr2m commented May 22, 2020

fixes #65

@ghost ghost added this to Inbox in JS May 22, 2020
@gr2m gr2m added the Type: Feature New feature or request label May 22, 2020
@ghost ghost moved this from Inbox to Features in JS May 22, 2020
@gr2m gr2m force-pushed the 65/retry-requests-on-401s-within-first-minute branch 2 times, most recently from cf180dd to f430913 Compare May 24, 2020 23:47
@gr2m gr2m changed the title 🚧 fix: retry requests on 401 response within first minute fix: retry requests on 401 response within first minute May 25, 2020
@gr2m
Copy link
Contributor Author

gr2m commented May 25, 2020

@steve-norman-rft can you test if this change resolves your problem? Note that you need to pass the authentication strategy and options to the constructor, in order for @octokit/auth-app to hook into the request life cycle, so that it can retry failed requests.

So instead of

const auth = createAppAuth({
  id: 10115,
  privateKey: `-----BEGIN RSA PRIVATE KEY-----\nMIIEowIBAAKCAQEAscPv+FbKL3o2C6f73Z7C/dB0txdTC9UeDYMBRkzbMXqv9D+i\nSjg8yyCIS1A1djAofLo/PGVlewjzlkHaa/yW38vmJVKscwwAwvJXteVhmnOPY3fV\nlqSZdZPxu4HGft2v7R0FGqh7Q8P4FGKgVhwd1NdJjvYdLYdDVd0LVGCTXa/Hsqyk\nqzm77FuG7YAc9mq1Lqp34+BZ/Z8U2OGdqvlLyzzTZMQp1K552uOtSFMS+hX/hrKd\nw3UPB//3BoA1ciYpDsY1pkOG1xREwq1nPdZkRJIofgYHOsFVg5eSsY0+FF6jMkF+\nn+3TcqH6G/0aLRaZ99dKM6fYYa/E9VXxW816uwIDAQABAoIBAEyM8gPhbAPx/uAa\nIM4ZFiMy53AI7UxFJEHxTlU1t5ahHLBzzrFjclqO0eKM0djpeCXuFlkt2PuYqTzl\ncufZyCbrqVodNgH9Az7wGXFNLDDU3sSY0DOlAiit4wU6J35ufNoBCzeloh9WfrhY\nmG88gGQtqZGUzo/Sld6d58kYJZ3L4rK3u0/rNl9tnx7h4vTv2QTCyri5jC/dm452\np+8taatxHS6cbuN6lThISdbt8jhYgdWiYWL2Dhch6oQ+JHeYeSoOFUZ+oPGn+d42\nH7WXEb0qH+cVMnaWGJrj87R1VdtEsA4MubO654bllnK5U7NaoGKkJptJaY0aokJl\nm2c/egECgYEA3kAnMkK4X8zYJJoNV9kNjk96PIC+mheBpTKNJWdNA/0rq7fJ1jV/\ncmnWEoJeGoVA8+3ZmMza7+ivtKgwKDedg0ztkY7k4/KQWxm+ye+XqXa2ukF72A/K\nDnG7kCuQHp2o5TsJFSaeSmXDLTcAetQiajM11i5nqd4wsRCG8WxbH4ECgYEAzMJz\nUqnBwsg7ktUwm6cuLtShrYWr6OxI+1wSxkH93/1IgAwZY9wN8zlFdln6eakAa79q\nWD2PWplIAgSKbQCB7Zggc91s5qG+rbep0jpkCRWOGaqPbm42eR0/WW/5JaBgezuy\n6H/IJjcTi3QzaDVIsZ26yTztjlWYhLVw/YhpODsCgYBDk7AXWTAkeO2Tm3/JIUc8\n6S+aq+7IfbM+3rMKF3HUb6tSqCxnxJZ+3G1p7VfdqnzIbp7GFivP3KloPed+owJy\nxPZLVu6D3OJFwPtA/WfY8C65TWXxmUNvxucn/AbzOC79nEiztK1Wo8CHw/ySXGQm\ndHG/MRb2EIgvnn3ZdH/0gQKBgAhevoYXiGMk+kJzUoxaVin16TIFr9RSrF0SE3Zl\nRjJ80RTi6brKSQuzQKKo5PNKNEkzRu6afZyvfWEPvHTeO3Fx8Ymq+IOpAvlo85kz\nZ9lzNx7XeQuDl9h1mJxYVm/yUV9YPmUArDbbs4HU2zEcVH9mOjd1tSRHXXe3Twoe\nJGb3AoGBAJV+mKlB0gW+SpYk/BUQIjOxy9qwll/Tmy970HHNPPQvO0DTMDDmAvnK\nFY7UCegUSJcUqJhK8wsNYLB8+FpkJDOszjcqkKx6Oswete7KRqOPaDJ7w21H+f8+\nMrFZSjyKULnm7+tgtL09Oh2LzqpUt8N/GAClmzA1dVV4MViM+wD8\n-----END RSA PRIVATE KEY-----\n`.replace(
    /\\n/g,
    "\n"
  ),
});

const installationAuthentication = await auth({
  type: "installation",
  installationId: 743367,
  refresh: true,
});

const PluginOctokit = Octokit.plugin(paginateRest, enterpriseCloud, retry);
const octokit = new PluginOctokit({
  auth: installationAuthentication.token,
});

Do

const PluginOctokit = Octokit.plugin(paginateRest, enterpriseCloud, retry);
const octokit = new PluginOctokit({
  authStrategy: createAppAuth,
  auth: {
    id: 123,
    privateKey: `...`,
    installationId: 12345
  }
});

gr2m added a commit to octokit/sandbox that referenced this pull request May 25, 2020
gr2m added a commit to octokit/sandbox that referenced this pull request May 25, 2020
@gr2m
Copy link
Contributor Author

gr2m commented May 25, 2020

Tested

@gr2m gr2m self-assigned this May 25, 2020
@ghost ghost moved this from Features to In progress in JS May 25, 2020
@gr2m gr2m force-pushed the 65/retry-requests-on-401s-within-first-minute branch from f430913 to fa52af4 Compare June 3, 2020 20:02
@gr2m gr2m merged commit 2db0104 into master Jun 3, 2020
JS automation moved this from In progress to Done Jun 3, 2020
@gr2m gr2m deleted the 65/retry-requests-on-401s-within-first-minute branch June 3, 2020 20:06
@github-actions
Copy link
Contributor

github-actions bot commented Jun 3, 2020

🎉 This PR is included in version 2.4.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

@steve-norman-rft
Copy link
Contributor

@gr2m Sorry for the delay, been off work for a few days and busy with other things.

At the moment I'm struggling to actually get the test script to fail, so far today I've made 500 authentication generation calls 2 seconds apart and it has yet to fail. Similar last week when I tried it. So hard to know if it has fixed the issue when I can't generate it again. Perhaps something has changed internally as well?

I will try updating and using the new auth model this afternoon and see if there are any issues though.

One side question from this is by changing to use the authentication strategy is there a way to get the token being generated out of the code so that I can cache it between runs. I understand that it can be cached by octokit in memory, but for some of my apps they are just jobs that are run adhoc or via cron so it would be nice to be able to cache the token somewhere. I had implemented it at support's request and currently securely cache them to a Postgres DB and only generate a token if there isn't a current one, or one about to expire. But with the authentication strategy I don't think the client can see the token at all?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New feature or request
Projects
No open projects
JS
  
Done
Development

Successfully merging this pull request may close these issues.

Handle 401 response when using installation token shortly after creation
2 participants