watch - handle annotation errors gracefully

[wingleung]

fixes #8842

[andyfeller]

@wingleung : thank you for your continued support! 🤗 aside from getting checks to turn green, I think these changes are pretty straight forward.

[andyfeller]

Aside from linting error here, I think all the changes make sense 👍

[wingleung]

oh 😅 I didn't have my linting set up locally, thanks for the heads up. fixed!

[andyfeller]

@wingleung : given that @williammartin has been working on this issue, I'm going to defer to him for the review on these changes as I think he had more specific ideas how this would go.

[williammartin]

Thanks for this PR. I have a couple of thoughts I'd like your input on. A couple of facts to make sure we're on the same page:

• Annotations are per job (so one run watch may have multiple annotations)
• Annotations are currently rendered last
• Fetching annotations can fail for many reasons

Previously, the first annotation fetch that failed would result in the CLI exiting with no other content rendering.

With this PR, if an annotation fetch fails, we will:

• Print that error message at the top
• Successfully render any annotation that were fetched before the failed fetch
• Do not render any annotations that would have been fetched after the failed fetch due to the break

So that leaves me with two questions:

• If the annotation fetch failed for a transient reason e.g. 500 rather than a 403 which should impact all requests, should we try to render all the successful annotations?
• Should we put the error message around the existing ANNOTATIONS section so that it is visible where the user would expect it?

I think the right thing to keep things simple is:

• Move the error below into the ANNOTATIONS section. I think the easiest way to do this is to move the if annotationErr != nil check to line 257 and to make an if else len(annotations) > 0 and print the header in both blocks.
• Always print the error even if there were successful fetches of annotations. This is consistent with what used to happen. If in the future people complain that they would have preferred more information, we can change it then.

Hope this makes sense, let me know if not!

[wingleung]

@williammartin yes, you're correct in your understanding what the PR introduces. thanks for thinking along!

I saw the break as a circuit breaker to prevent repeated requests to the backend in the event of an error, thereby allowing the backend some breathing room. But maybe I overlooked some better UX solutions.

if you want to try and fetch annotations of every job regardless of previous errors, it could lead to unnecessary calls to the backend. especially in the span of a for loop.
However 5xx and 404 might be triggered (unwillingly) by content errors instead of credentials or backend health so I agree it would be nice to show the other successful annotations given it's ok to step away from the circuit breaker.

short term

I'll update the PR later this week and add your suggestion of moving the annotation errors to the ANNOTATIONS section.
At this stage of the PR, I would remove the break and collect the annotation errors in an array which I would print out in the ANNOTATIONS section.

The caveat is that we would potentially do unnecessary calls to the platform.

long term?

Maybe for the long term, we should also consider rethinking the approach of fetching annotations within a for loop. It might be more efficient to perform a single GraphQL query to fetch all annotations for a given list of jobs. And let platform handle missing annotations and server errors while gracefully returning valid annotations.

[williammartin]

if you want to try and fetch annotations of every job regardless of previous errors, it could lead to unnecessary calls to the backend. especially in the span of a for loop.

No, I don't want this, sorry for confusing you. I think we are in agreement. The behaviour today is "if there is any error fetching any annotations then exit the CLI". I'm content to continue with that, except that instead of exiting, we print the error under the annotations section.

If someone in the future says "well, only one annotation fetch failed, why can't you show me the rest?" then we can consider changing it but since no one has asked for that yet then I don't think we need to make our lives harder to support it.

I also agree with you that in the long term if we can make this more performant we should. Even now we should probably be making concurrent requests for uncached annotations. It seems unnecessary to do them all sequentially. However, let's address that in a future PR.

Dank je wel!

[wingleung]

PR updated!

So I've set up the possibility of fetching every jobs annotation regardless of error in commit 👉 751f37f

output

Triggered via push about 59 minutes ago

JOBS
X sad job in 4m34s (ID 20)
  ✓ barf the quux
  X quux the barf
✓ cool job in 4m34s (ID 10)

ANNOTATIONS
! failed to get annotations for job 'sad job' (20): HTTP 403 (https://api.github.com/repos/OWNER/REPO/check-runs/20/annotations)
- the job is happy
  cool job: blaze.py#420


To see what failed, try: gh run view 1234 --log-failed
View this run on GitHub: https://github.com/runs/1234

Then I pushed a simple commit to break on the first annotation error so we won't fetch any more annotations 👉 9e7e76a

output

Triggered via push about 59 minutes ago

JOBS
X sad job in 4m34s (ID 20)
  ✓ barf the quux
  X quux the barf
✓ cool job in 4m34s (ID 10)

ANNOTATIONS
! failed to get annotations for job 'sad job' (20): HTTP 403 (https://api.github.com/repos/OWNER/REPO/check-runs/20/annotations)

To see what failed, try: gh run view 1234 --log-failed
View this run on GitHub: https://github.com/runs/1234

[williammartin]

@wingleung thanks for your changes, they got me thinking some more about this and I changed my mind about something (sorry!). I pushed a commit on top of your changes because I thought it would be easier to talk about the code specifically than abstractly. We can always revert it if we don't like it.

I decided that I would rather we only address 403 status codes as "informative". That is because if you have a fine-grained PAT, there is nothing you can do about it, you can't add a permission and you can't just re-run. In other cases you can take an action (even if that action is re-running). Though I'm open to the idea of "graceful degradation", it's not something I think we do anywhere else in the CLI and I would rather collect more evidence on how it should work (like this issue) before committing to any particular direction. Limiting the change to just 403 responses is less risky.

One final step we could take here is to interrogate the token and only make it gracefully degrade if it is a v2 PAT. I chose not to do this yet because I wanted to discuss the larger approach with you first.

I've also reverted a stylistic change you made because it's potentially breaking for non-interactive scripts. Even if we were to do it, it would be better tracked as a separate issue.

[wingleung]

@williammartin thanks for the code to make it more visual for me 👍 I understand we shouldn't touch too many things that could be considered out of scope for this ticket.

however, if people run into another error on annotations, I think they would still want to be able to see the primary content, in this case the jobs. There would be another ticket to ask for another exception to the general rule of stopping the cli on errors. should there be another error, I guess it would be good if we're notified in the form of a ticket here. But then we need to make the decision between long term ux (full graceful) vs observability (partial graceful).

I'm ok with either 😄 at least we thought well about it and and can explain in the future why we did it this way.

about checking the token before handling the error gracefully. according to that principle we would also need to remove it again if PAT does support the permission. so it could be unclear in the future what the general idea was.

you're right about the stylistic tweaks, it seemed small but ideally it's a different ticket idd ✅

[williammartin]

I'm ok with either 😄 at least we thought well about it and and can explain in the future why we did it this way.

Totally. The different approaches in your PR were very useful to help me think about how to proceed.

according to that principle we would also need to remove it again if PAT does support the permission. so it could be unclear in the future what the general idea was.

I don't understand what you mean here, sorry! When you say "remove it again" what do you mean by "it"?

[wingleung]

according to that principle we would also need to remove it again if PAT does support the permission. so it could be unclear in the future what the general idea was.

I don't understand what you mean here, sorry! When you say "remove it again" what do you mean by "it"?

Sorry, by "it" I mean the extra check on PAT version that would trigger the graceful flow.

if err != shared.ErrPersonalAccessTokenV2MissingPermissions {
	return fmt.Errorf("failed to get annotations: %w", err)
}

missingAnnotationsPermissions = true

...

if missingAnnotationsPermissions {
	fmt.Fprintln(out)
	fmt.Fprintln(out, cs.Bold("ANNOTATIONS"))
	fmt.Fprintln(out, "requesting annotations returned 403 Forbidden as the token does not have sufficient permissions. Note that it is not currently possible to create a fine-grained PAT with the `checks:read` permission.")
} else if len(annotations) > 0 {

...

If we check the PAT version and only use the graceful flow for PATv2, we're acknowledging that since PAT v2 cannot grant access to annotations, we will handle this gracefully by showing an informative message instead of stopping the process.

This approach tightly couples our logic to the restrictions of the tokens. However, if PAT v2 in the future does decide to support access to annotations, there is no reason to maintain a graceful flow specifically for PAT v2. In such a case, all errors should result in an exit, as the user can address the PAT v2 issue themselves.

[williammartin]

Yep, that all makes sense to me. I think it's the ideal situation actually because it becomes extremely clear under what circumstances, and what code can be removed in order to simplify the flow again.

[wingleung]

👍 ok clear, then the only concern is that we need to remember the graceful flow should be cleaned up when PAT v2 supports annotations (code comments?). And we will need to be notified somehow when annotations support has landed in PAT v2.

Don't know if that's something you can link internally at github? For example issue about annotation support in user services would have a follow up issue in cli to clean up the graceful flow. Or just wait until someone creates a issue about it 😅

[williammartin]

Sorry for the delay in response @wingleung.

Don't know if that's something you can link internally at github?

This is already linked to internally but you can't see it unfortunately.

In terms of the v2 PAT check, I had a look at it and I didn't see an option that I particularly liked. I'd rather merge this as is, allowing Legacy PATs to also gracefully degrade and prioritise that work another time if it becomes important.

[williammartin]

LGTM thank so much for your work on this!

Dismissing because it was a linter error that was addressed.