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

Add an automatic version check. #408

Closed
wants to merge 1 commit into from
Closed

Add an automatic version check. #408

wants to merge 1 commit into from

Conversation

parkr
Copy link
Contributor

@parkr parkr commented Feb 8, 2017

Ensuring that you're up to date with the latest github-pages version means you'll be accurately building your site locally to match the eventual Pages production deploy.

@benbalter, what do you think about this?

@benbalter
Copy link
Contributor

I like it, but first and foremost, this can't ever be a blocker. If I don't have internet, GitHub Pages is down, the JSON is invalid, I have the wrong version, etc. it should silently fail. I believe we recently used a DNS check to quickly tell if the user had internet access. Perhaps we should do that here as well, to fail as quickly as possible (rather than adding a 30 second timeout).

Jekyll.logger.error "GitHub Pages:", "You are using an outdated version of github-pages."
Jekyll.logger.error "", "Upgrade to version '#{production_ghp_version}' to ensure your local builds"
Jekyll.logger.error "", "will accurately reflect what is deployed when you push to GitHub."
return false
Copy link
Contributor

Choose a reason for hiding this comment

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

We should explain how to update and ideally link to the help docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

private

# rubocop:disable Lint/HandleExceptions
def http_get_with_retries(url)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to retry 5 times?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using open-uri, I have experienced about half of the time pulling in this file and getting nothing back with some kind of server error. I figured we should retry just in case the user is on an unreliable connection such as coffeeshop wifi or airport wifi.

@parkr
Copy link
Contributor Author

parkr commented Feb 8, 2017

this can't ever be a blocker

Agreed – a reliable internet-connectivity check is required. A DNS check should work, though a TCP connection would be an even better bet of ensuring our connection is capable of this kind of transaction.

rather than adding a 30 second timeout

2 second timeout? This runs once right at the beginning.

@kvz
Copy link

kvz commented Feb 15, 2017

Stumbled upon this. In case it helps: I've seen in the Node.js ecosystem the way is to check during/after the main job is done, saving new version information to disk. This can fail all it wants. But if the next execution does find such a file on disk with a higher version number, it is proposed to the user.

@adamvoss
Copy link

adamvoss commented Jun 29, 2017

2 second timeout? This runs once right at the beginning.

If I am doing something where watch does not work and I need to restart Jekyll to rebuild, 2 seconds is an awfully long time to need to wait on every start. I think I'd want a setting to disable this check.

@parkr parkr closed this Mar 20, 2018
@parkr parkr deleted the version-check-again branch March 20, 2018 01:24
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.

4 participants