feat: Refresh app tokens

[sicarul]

When running long initial syncs using Github app private keys, the tokens expire in 1 hour, which makes the process fail after this timespan. This PR separates the app tokens into a new function refresh_app_token that can get called again in get_next_auth_token, i've tested it for my use case and it no longer fails after 1 hour.

[ericboucher]

Thanks @sicarul for your PR. I'll have a look and test it. In the meantime, could you make sure mypy and other tests are passing?

[sicarul]

I've fixed the mypy error, it seems it doesn't like me defining the type of the variable on init for some reason

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[ericboucher]

This is a change in behavior as it returns only this token now.

[sicarul]

refresh_app_token is a new function i created, can you explain how is this a change in behavior? Maybe it's confusing because of how the diff got the change?

prepare_tokens still returns all tokens as it did, it just doesn't do the private key part anymore, which is now handled by this function when all other tokens are used up.

        if self.active_token is None:
            self.active_token = self.refresh_app_token()

[ericboucher]

Indeed! I think the fact that this token is now handled outside of "prepare_tokens" is a bit confusing.

[ericboucher]

Thanks @sicarul for opening this PR and sorry it took so long to get back to you.

The logic looks great overall. However, I think it would make more sense to add the new timestamp check within TokenRateLimit or something along these lines. This way, you will be able to keep running the tap with multiple token types if needed and leverage the existing calls to "is_valid" and other rate-limiting checks.

Let me know what you think!

[sicarul]

Hey @ericboucher i don't understand how would the timestamp check work in that class, it does not handle any kind of token refresh and it's used for all types of tokens, by putting the logic there it seems i would be generating a spaghetti of code to me, but maybe i'm just not understanding how do you want me to change this.

[ericboucher]

@sicarul I was thinking that you could either augment the TokenRateLimit class, or use it as a subclass of a new AppTokenRateLimit class which would:

• add the necessary info to refresh the token (github_app_id, github_private_key, github_installation_id)
• override the is_valid function to handle the refresh if necessary

I believe this would make the code a bit cleaner and keep the token logic in one place. If you prefer your current approach, make sure to add sufficient comments in the code to guide users/readers and make it clear how things are split between normal tokens and app tokens

[laurentS]

If I may chime in, I did find it a bit difficult to follow the overall logic (some high level comments would help a lot). Am I getting this right?

• the tap first tries to load GITHUB_TOKEN... tokens passed as env vars, then loads a private key based one, only if there are no other tokens (during init). So you can't cumulate API tokens+private key anymore (self.token_map never gets the app token now)?
• then as it uses tokens, we call get_next_auth_token if we run out of quota (which we know from getting an error from the server), to rotate the token (but this excludes the private key based one)
• then when getting auth_headers we would rotate the private key based token if that's what we're using.

If the above is correct:

• I find it confusing that there is a non-obvious side effect in auth_headers (of refreshing the token)
• I think it's a regression if we can't use both types of tokens together anymore (we might want to cumulate limits from multiple accounts).

[sicarul]

Got it, i'm going on a vacation tomorrow until July but when i come back i can check this feedback and apply it to get a better version out if it's fine with you all!

@laurentS sorry if it's not clear, but this version will still use both types of tokens as i implemented it.

[sicarul]

Closing since this is being fixed in the new PR: #289