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

ignore github 403s #7616

Closed
wants to merge 1 commit into from
Closed

ignore github 403s #7616

wants to merge 1 commit into from

Conversation

susanev
Copy link
Contributor

@susanev susanev commented Jun 3, 2022

website notifications is full of these
can we ignore them for now?

to clarify, there is nothing wrong with the github links, but the link checker is reporting them as 403s in the website notifications channel because of how the link checker was written, 20 or something a day

@github-actions
Copy link

github-actions bot commented Jun 3, 2022

Your site preview for commit 0e765e1 is ready! 🎉

http://pulumi-docs-origin-pr-7616-0e765e19.s3-website.us-west-2.amazonaws.com.

Copy link

@pulumi pulumi bot left a comment

Choose a reason for hiding this comment

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

🍹 The Update (preview) for stack pulumi/www.pulumi.com/production was successful.

Resource Changes

    Name                  Type                                      Operation
~   cdn                   aws:cloudfront/distribution:Distribution  update
*   origin-bucket-policy  aws:s3/bucketPolicy:BucketPolicy          replaced

@@ -306,6 +306,9 @@ function excludeAcceptable(links) {
// intelligently, but we can come back to that in a follow up.
.filter(b => !(b.reason === "HTTP_429" && b.destination.match(/github.com|npmjs.com/)))

// Ignore GitHub 403s
.filter(b => !(b.reason === "HTTP_403" && b.destination.match(/github.com/)))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe we should ignore all 403s not just github ones 🤷🏻

Copy link
Contributor

Choose a reason for hiding this comment

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

Before we just go ignoring, do we have some idea why these are returning 403 to begin with?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i thought you wrote why in slack...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you have another idea pls lets do that, the noise of the 403s is preventing us from seeing and fixing the 404s.

Copy link
Contributor

Choose a reason for hiding this comment

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

That was one possibility, yeah, based on past experience. Has anyone looked more closely at these particular ones yet?

Copy link
Contributor

@cnunciato cnunciato Jun 6, 2022

Choose a reason for hiding this comment

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

Bit of Googling led me to this issue, which suggests adding an accept-encoding header can fix this, and local tests appear to confirm that me. So I'd probably explore adding that header first. A new line with an explanatory comment below this (similar) addition would probably be the way to do that: https://github.com/pulumi/docs/blob/master/scripts/link-checker/check-links.js#L52-L53 You can then run the checker locally against a page you know contains errant links like this:

$ ./scripts/link-checker/check-links.sh "page" "https://www.pulumi.com/blog/managing-github-webhooks-with-pulumi/"

@susanev susanev closed this Jun 6, 2022
@github-actions
Copy link

github-actions bot commented Jun 6, 2022

Site previews for this pull request have been removed. ✨

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.

2 participants