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

attestation: handle bad configurations better #17800

Merged
merged 1 commit into from
Jul 18, 2024
Merged

Conversation

Bo98
Copy link
Member

@Bo98 Bo98 commented Jul 18, 2024

  • After Ensure early installation of gh for attestations #17760, we seem to now be forcing installing gh for everyone which I didn't initially realise based on the PR description. This can be slow for unsupported configurations as they would need to build Go from source so disable attestations for the OS.unsupported_configuration? case. (Though IMO maybe worth thinking whether we actually want to be forcing installs to 15% of users.)
  • Set GH_HOST explicitly which should avoid the need for gh auth switch
  • Silently handle 401 bad auth for now. Removing auth is a hard blocker for wider roll out so I don't think we should pain users in the meantime. We've been getting multiple reports a day on this issue and that number doesn't seem to be going down.

@Bo98 Bo98 requested a review from woodruffw July 18, 2024 15:19
Copy link
Member

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thought, otherwise LGTM! Thanks @Bo98!

@Bo98 Bo98 merged commit 8d6ba63 into master Jul 18, 2024
24 checks passed
@Bo98 Bo98 deleted the attestations-bad-config branch July 18, 2024 15:53
@carlocab
Copy link
Member

I wonder if we should just skip HOMEBREW_GITHUB_API_TOKEN for attestations and rely solely on gh auth token instead. (And error only if HOMEBREW_VERIFY_ATTESTATIONS is specifically requested.)

@Bo98
Copy link
Member Author

Bo98 commented Jul 18, 2024

Potentially, though it's all silenced now anyway and in reality we need the auth requirement to be dropped entirely for the beta to progress any further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants