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

Delete build-system/tasks/package.json, whitelist heroku page #13295

Merged
merged 2 commits into from
Feb 6, 2018
Merged

Delete build-system/tasks/package.json, whitelist heroku page #13295

merged 2 commits into from
Feb 6, 2018

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Feb 6, 2018

This PR contains two gulp check-links fixes:

  1. build-system/tasks/package.json is superfluous, since the only dependency it contains already exists in ampproject/amphtml/package.json. The file was added in Add a PR check test to catch dead links during docs changes #9099, back when I didn't know what I was doing :)
  2. The link checker frequently trips up on http://amphtml-nightly.herokuapp.com/, resulting in several failed builds on Travis. Since this is a well known page that's unlikely to ever be a truly dead link, we whitelist it during gulp check-links.

@rsimha
Copy link
Contributor Author

rsimha commented Feb 6, 2018

/to @erwinmombay @choumx

@rsimha rsimha changed the title Delete build-system/tasks/package.json Delete build-system/tasks/package.json, whitelist heroku page Feb 6, 2018
@rsimha
Copy link
Contributor Author

rsimha commented Feb 6, 2018

/to @danielrozenberg

@@ -143,6 +143,10 @@ function filterWhitelistedLinks(markdown) {
// Links inside a <code> block (illustrative, and not always valid)
filteredMarkdown = filteredMarkdown.replace(/<code>(.*?)<\/code>/g, '');

// The heroku nightly build page is not always acccessible by the checker.
filteredMarkdown = filteredMarkdown.replace(
/\(http:\/\/amphtml-nightly.herokuapp.com\/\)/g, '');
Copy link
Member

Choose a reason for hiding this comment

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

Replace http with https?

Copy link
Contributor Author

@rsimha rsimha Feb 6, 2018

Choose a reason for hiding this comment

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

The original URL in DEVELOPING.md is http, so we must whitelist it that way. Also, I'm not sure that changing the original URL will make this less flaky, so I think whitelisting is the way to go. The check is meant to catch other broken links, due to defunct external sites, or bad relative links.

Copy link
Member

Choose a reason for hiding this comment

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

https? (question mark included) means it'll catch both http and https, in case someone changes the documentation later :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha! Right now, we only have the one place where we document the URL, so I'll keep it this way for simplicity.

@rsimha rsimha merged commit 99a0424 into ampproject:master Feb 6, 2018
@rsimha rsimha deleted the 2018-02-06-RemovePackage.json branch February 6, 2018 17:58
protonate pushed a commit to protonate/amphtml that referenced this pull request Feb 26, 2018
protonate pushed a commit to protonate/amphtml that referenced this pull request Mar 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants