Modify graphql token logic to include refresh #2475
Conversation
jamengual
added
waiting-on-review
|
@rayterrill take a look at this: #2128 Another thing to note is if in the docs the scopes are correct, AFAIk Thanks for the contribution again. |
|
Doing a little more testing around this this morning to see if we can handle this a little slicker. |
|
@jamengual I think this is the right library to use. I'm not sure what to check on the scopes side - this is working for me with the "normal" Atlantis scopes, and the addition of the "administration: read only" which I'm not sure where we'd need to document this. Potentially this graphql token piece could be refactored, but this is working for me, and it's a pretty simple change. Hesitant to break this apart too much. @stasostrovskyi i know you were running into this pretty frequently - any chance you can test with this code and see if the graphql issues you were hitting go away? |
|
So we basically create a new client on each query to workaround the refresh token problem? I can try and verify this fix, same company with @stasostrovskyi |
|
For now @ysoldak. I'm working on another update that will actually refresh the token as-needed vs generating a new one each query - might need some deeper changes to the underlying abstractions. |
|
I'm testing this fix locally now instead. Seem to be working actually. I.e. token is refreshed as it should. |
|
We shall run this (see prev comment) for some days on our prod and if no prob, I can file a PR then. |
|
That's pretty much identical to what I have running locally as well - getting that transport in there allows the underlying library to refresh the token as needed. |
|
Yes, the problem as I see it is in |
| //iterate over the runs inside the suite | ||
| suite, _, err := g.client.Checks.ListCheckRunsCheckSuite(context.Background(), repo.Owner, repo.Name, *c.ID, nil) | ||
| if err != nil { | ||
| return false, errors.Wrap(err, "getting check runs for check suite") | ||
| } | ||
|
|
||
| for _, r := range suite.CheckRuns { | ||
| fmt.Printf("Looking at check run %s\n", *r.Name) | ||
| //check to see if the check is required | ||
| if isRequiredCheck(*r.Name, required.RequiredStatusChecks.Contexts) { |
There was a problem hiding this comment.
This code will crash if no required checks are configured for the repo. While feature flag kind of implies that at least one required check should be set - atlantis/apply - it's completely plausible to have 1 or 2 repos in organization misconfigured or deliberately configured in a different way. If there are no required checks it means that we don't even need to go through CheckRuns.
I understand it's out of scope of this PR, but feels like the best place to point it out :)
There was a problem hiding this comment.
I'm not sure what you mean @stasostrovskyi - I just tested this again with no required checks configured and it works fine? It should just pass right through that section if none of the checks are required?
There was a problem hiding this comment.
This is what we got when running without required checks
Error: goroutine panic. This is a bug.
runtime error: invalid memory address or nil pointer dereference
runtime/panic.go:260 (0x44c9d5)
runtime/signal_unix.go:835 (0x44c9a5)
github.com/runatlantis/atlantis/server/events/vcs/github_client.go:357 (0xa89acc)
github.com/runatlantis/atlantis/server/events/vcs/github_client.go:430 (0xa8a197)
github.com/runatlantis/atlantis/server/events/vcs/instrumented_client.go:179 (0xa90beb)
github.com/runatlantis/atlantis/server/events/vcs/proxy.go:72 (0xa925e4)
github.com/runatlantis/atlantis/server/events/vcs/pull_status_fetcher.go:28 (0xa92e65)
github.com/runatlantis/atlantis/server/events/apply_command_runner.go:109 (0xc72835)
github.com/runatlantis/atlantis/server/events/command_runner.go:296 (0xc77443)
runtime/asm_amd64.s:1594 (0x467ce0)
I haven't debugged it but it's either required or RequiredStatusChecks are nil in the case when there are no required checks.
There was a problem hiding this comment.
GitHub probably returns nil ("null" in JSON) instead of an empty list as the code expects.
|
it looks like an easy fix and since we are here......we can do it in this
PR or maybe ***@***.*** or Ray might want to give it a try in another
PR?*
…On Tue, Aug 30, 2022 at 9:58 AM Stas Ostrovskyi ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In server/events/vcs/github_client.go
<#2475 (comment)>:
> //iterate over the runs inside the suite
suite, _, err := g.client.Checks.ListCheckRunsCheckSuite(context.Background(), repo.Owner, repo.Name, *c.ID, nil)
if err != nil {
return false, errors.Wrap(err, "getting check runs for check suite")
}
for _, r := range suite.CheckRuns {
- fmt.Printf("Looking at check run %s\n", *r.Name)
//check to see if the check is required
if isRequiredCheck(*r.Name, required.RequiredStatusChecks.Contexts) {
This code will crash if no required checks are configured for the repo.
While feature flag kind of implies that at least one required check should
be set - atlantis/apply - it's completely plausible to have 1 or 2 repos
in organization misconfigured or deliberately configured in a different
way. If there are no required checks it means that we don't even need to go
through CheckRuns.
I understand it's out of scope of this PR, but feels like the best place
to point it out :)
—
Reply to this email directly, view it on GitHub
<#2475 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAQ3ERE33QU3MJAJTIHPZBTV3Y4STANCNFSM57YB4EIA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
I pulled the change to use the transport into my local copy and it's working correctly for me - passed the 1 hour token expiry mark and I'm able to make graphql calls succesfully. |
|
Great, shall I make a PR? |
|
A new PR is fine with me @ysoldak - I can close this one out. Can you include the removal of the extra print statements I have in this PR? |
yes, no prob, will cherry-pick. |
#2285