Problem with Github Token logic - Error 403 (not logged) vs Error 500 (logged)

[swdbo]

Hello !

Thanks a lot for your work.

I've found that for some URLS, tinuous fetch fails to handle the "Must have admin rights" case, with multiple retries and an exit error.

Expected behaviour : skip this URL and continue to fetch remaining logs.

To reproduce :

403 with "message": "Must have admin rights to Repository."

curl -i \                                                                   
    https://api.github.com/repos/ProjectOpenSea/opensea-js/actions/runs/1868323967/logs

500 with "message": "Failed to generate URL to download logs"

curl -i -H "Authorization: token ghp_REDACTED" \
    https://api.github.com/repos/ProjectOpenSea/opensea-js/actions/runs/1868323967/logs

Have a nice day.

[jwodder]

@yarikoptic What exactly should we do about errors like this? Currently, I believe the only errors that are handled by skipping downloads are when a request for GitHub logs returns 404 (when a workflow failed to run) or 410 (when the logs have expired).

[yarikoptic]

Expected behaviour : skip this URL and continue to fetch remaining logs.

I might be missing the point, but I would say that it would not be expected behavior for me. For me crashing with some error "Requires special rights, use another token or set variable ... to ... . Original error was : ...." would be more "expected" and avoid surprise ("where the heck my logs?") later.
So I think, @jwodder, it would be nice if we allow (configuration variable) to skip some (but not all) errors like this. How should we spec for them? I think making generic needing_admin_rights: skip would be too crude. Thinking of may be making some more flexible, per provider (github, circle, etc) config like

skip_errors:
  - code: 403
    message: "Must have admin rights .*"

WDYT?

[jwodder]

@yarikoptic I think specifying individual errors to skip on is overkill. Anyone who wants something like that would probably be satisfied with a single option for skipping any & all logs that fail to download because of some HTTP status error.

[yarikoptic]

hm, but not all errors are alike -- we have spent a good number of iterations to solidify operation while figuring out where to retry but still keep running into some new ones from time to time. Just enabling "skip then all" could hide grave problems... For such folks suggested syntax could be

skip_errors:
  - message: .*

but I would never use such setting. What about you @swdbo -- really just want to skip anything you fail to download right at this point in time ?

[swdbo]

Many interesting ideas here.
In my case, I would like to be able to skip anything that fails.
We could maybe have a log file to track errors ?

[yarikoptic]

@swdbo we should definitely log those. Getting back to original problem -- does it fetch some other logs (or could some actions logs be available only to admins and others "publicly")? or if nothing is fetched, why not to just disable it for github actions?

@jwodder I still think that making filtering of errors to ignore would be beneficial. Would that idea above easy to implement given current code structure?

[swdbo]

or could some actions logs be available only to admins and others "publicly"
@yarikoptic Yes, I remember that's exactly what happens with the ProjectOpenSea repo I linked : everything was going fine until that specific log error made the whole process stop. So to me, it should simply skip the log.

[jwodder]

@yarikoptic I don't think making the user specify individual errors to filter would be user friendly. Here's a better question to consider: Are there any circumstances in which we actually want failures to fetch logs to stop the program?

[yarikoptic]

Are there any circumstances in which we actually want failures to fetch logs to stop the program?

I think it should be the default behavior for any program to stop on an error which we have not decided consciously how to handle yet. That is why I do set -eu in all my bash scripts ;) So we want to stop for any error which we do not know yet to be benign/which would go away/need to have some special handling/... That is why we had been catching such use cases and adding retries etc. In this particular use case it is IMHO up for a user to decide on what to do for such type of error, e.g.:

• instruct program to just skip it
• may be use another auth token with wider range of permissions
• go into settings for the repo and adjust them to gain permission
• talk to the repo owners to get allowed

So it is a decision for the user, not us, to make and possibly configure con/tinuous to reflect this decision to skip. If we just silently ignore it -- user is not given a chance for an alternative handling and possibly would not even know for years to come that he/she is not collecting all logs.

[jwodder]

@swdbo Could you elaborate on exactly what's happening with the errors you're seeing? Your first curl command (without a token) is not an accurate reproduction of anything tinuous does, as tinuous always requires a token when interacting with GitHub. Moreover, I don't see anything in the tinuous code that would cause a 403 to be retried unless it was accompanied by an "API rate limit exceeded" message. As for the 500 error, that appears to be a problem on GitHub's end, in which case I believe retrying and eventually giving up with an error is the correct thing to do.

[swdbo]

Yes of course, I'll rephrase.
I just want to fetch all the logs I'm allowed to consult.
It can be private logs or public logs - if I have no rights associated with my token.

The point I was trying to make with the initial post is that tinuous retries to fetch logs that are protected (403 error), and then exit. I think it shouldn't block the all process but only skip the logs that cannot be fetched (with a logs_error file).

---

I ran into another example of this error handling behaviour on Travis this time : a 404 error also makes tinuous fail.
requests.exceptions.HTTPError: 404 Client Error: Not Found for url: https://api.travis-ci.com/job/552412604/log.txt

Command :
tinuous --env .env --config tinuous.yaml fetch

File tinuous.yaml content :

repo: paypal/butterfly
vars:
  path_prefix: "results/{year}//{month}//{day}/{ci}/{type}"
  build_prefix: "{path_prefix}/{type_id}/{build_commit[:7]}"
ci:
  github:
    paths:
      logs: "{build_prefix}/{wf_name}/{number}/logs/"
      artifacts: "{build_prefix}/{wf_name}/{number}/artifacts/"
      releases: "{path_prefix}/{release_tag}/"
  travis:
    paths:
      logs: "{build_prefix}/{number}/{job}.txt"
since: 2021-01-20T00:00:00Z

[yarikoptic]

@yarikoptic I think specifying individual errors to skip on is overkill. Anyone who wants something like that would probably be satisfied with a single option for skipping any & all logs that fail to download because of some HTTP status error.

I think our troubleshooting/problem in #150 with triggering 500 responses from github falls into yet another use case of wanting to ignore only some errors.