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

TravisLintBear: Check for internet connection #1982

Merged
merged 1 commit into from Aug 17, 2017
Merged

Conversation

@yash-nisar
Copy link
Member

@yash-nisar yash-nisar commented Aug 6, 2017

Add a check for internet connection in the check_prerequisites
method without which the bear will fail to run.

Fixes #1978

For short term contributors: we understand that getting your commits well
defined like we require is a hard task and takes some learning. If you
look to help without wanting to contribute long term there's no need
for you to learn this. Just drop us a message and we'll take care of brushing
up your stuff for merge!

Checklist

  • I read the commit guidelines and I've followed
    them.
  • I ran coala over my code locally. (All commits have to pass
    individually.
    It is not sufficient to have "fixup commits" on your PR,
    our bot will still report the issues for the previous commit.) You will
    likely receive a lot of bot comments and build failures if coala does not
    pass on every single commit!

After you submit your pull request, DO NOT click the 'Update Branch' button.
When asked for a rebase, consult coala.io/rebase
instead.

Please consider helping us by reviewing other peoples pull requests as well:

The more you review, the more your score will grow at coala.io and we will
review your PRs faster!

@yash-nisar
Copy link
Member Author

@yash-nisar yash-nisar commented Aug 6, 2017

@yash-nisar yash-nisar force-pushed the yash-nisar:travis-bug branch from 6719c14 to 9407509 Aug 6, 2017
@gitmate-bot gitmate-bot added size/M and removed size/S labels Aug 6, 2017
@yash-nisar yash-nisar force-pushed the yash-nisar:travis-bug branch from 9407509 to b23963e Aug 6, 2017
m.head(TravisLintBear.check_connection_url,
status_code=200)
self.assertTrue(TravisLintBear.check_prerequisites())
m.head(TravisLintBear.check_connection_url,

This comment has been minimized.

@SanketDG

SanketDG Aug 6, 2017
Member

newline before this for readability

@@ -25,6 +28,31 @@ class TravisLintBear:
CAN_DETECT = {'Formatting', 'Syntax'}
SEE_MORE = 'https://docs.travis-ci.com/user/travis-lint'

# IP Address of www.google.com
check_connection_url = 'http://216.58.218.174'

This comment has been minimized.

@SanketDG

SanketDG Aug 6, 2017
Member

capitals?

This comment has been minimized.

@SanketDG

SanketDG Aug 6, 2017
Member

since this is also constant

@yash-nisar yash-nisar force-pushed the yash-nisar:travis-bug branch from b23963e to f0d2772 Aug 6, 2017
@yash-nisar
Copy link
Member Author

@yash-nisar yash-nisar commented Aug 6, 2017

@SanketDG Fixed, thanks 👍

@yash-nisar yash-nisar force-pushed the yash-nisar:travis-bug branch from f0d2772 to d76b638 Aug 6, 2017
code = cls.get_status_code(
cls.CHECK_CONNECTION_URL, cls.DEFAULT_TIMEOUT)
if which('travis') is None: # pragma: no cover
return ('Please ensure that travis has been installed. Refer to'

This comment has been minimized.

@Makman2

Makman2 Aug 6, 2017
Member

missing space after "to"

if which('travis') is None: # pragma: no cover
return ('Please ensure that travis has been installed. Refer to'
'https://docs.travis-ci.com/user/travis-lint'
'#Command-line-Validation for more details.')

This comment has been minimized.

@Makman2

Makman2 Aug 6, 2017
Member

Can't we use the check of the base class? This message is probably better than the default one, however optimally requirements will take this over^^

@yash-nisar yash-nisar force-pushed the yash-nisar:travis-bug branch from d76b638 to 80e3307 Aug 6, 2017
@gitmate-bot gitmate-bot added size/S and removed size/M labels Aug 6, 2017
@yash-nisar
Copy link
Member Author

@yash-nisar yash-nisar commented Aug 7, 2017

@Makman2 Updated.

code = cls.get_status_code(
cls.CHECK_CONNECTION_URL, cls.DEFAULT_TIMEOUT)
if which('travis') is None: # pragma: no cover
return super().check_prerequisites()

This comment has been minimized.

@Makman2

Makman2 Aug 7, 2017
Member

this should be done before every other checks.
-->

base_check = super().check_prerequisites():
if base_check is not True:  # <-- you also don't need the `which('travis')` then, because this is done by super().check_prerequisites()
    return base_check

# Now the internet check...

This comment has been minimized.

@yash-nisar

yash-nisar Aug 9, 2017
Author Member

Haha, I was drunk at that time 😖

@@ -25,6 +28,29 @@ class TravisLintBear:
CAN_DETECT = {'Formatting', 'Syntax'}
SEE_MORE = 'https://docs.travis-ci.com/user/travis-lint'

# IP Address of www.google.com
CHECK_CONNECTION_URL = 'http://216.58.218.174'

This comment has been minimized.

@Makman2

Makman2 Aug 7, 2017
Member

What address does travis lint require? Not the travis-ci.org address?

This comment has been minimized.

@yash-nisar

yash-nisar Aug 10, 2017
Author Member

The travis gem includes both a command line client and a Ruby library to interface with a Travis CI service. Both work with travis-ci.org, travis-ci.com or any custom Travis CI setup you might have. Check out the installation instructions to get it running in no time.

Yeah, the travis-ci.org address. Changing it right away 👍

@@ -25,6 +28,29 @@ class TravisLintBear:
CAN_DETECT = {'Formatting', 'Syntax'}
SEE_MORE = 'https://docs.travis-ci.com/user/travis-lint'

# IP Address of www.google.com
CHECK_CONNECTION_URL = 'http://216.58.218.174'
DEFAULT_TIMEOUT = 15

This comment has been minimized.

@Makman2

Makman2 Aug 7, 2017
Member

what's the default timeout of requests? Maybe we should just use that

timeout=timeout).status_code
return code
except requests.exceptions.RequestException:
pass

This comment has been minimized.

@Makman2

Makman2 Aug 7, 2017
Member

explicitly return None here

try:
code = requests.head(url, allow_redirects=False,
timeout=timeout).status_code
return code

This comment has been minimized.

@Makman2

Makman2 Aug 7, 2017
Member

why not just

return requests.head(...).status_code

? :)

try:
code = requests.head(url, allow_redirects=False,
timeout=timeout).status_code
return code

This comment has been minimized.

@Makman2

Makman2 Aug 7, 2017
Member

What if you actually receive an error code?

This comment has been minimized.

@yash-nisar

yash-nisar Aug 10, 2017
Author Member

Fixed 👍

@yash-nisar yash-nisar force-pushed the yash-nisar:travis-bug branch from 80e3307 to 1b6ec14 Aug 10, 2017
@yash-nisar
Copy link
Member Author

@yash-nisar yash-nisar commented Aug 10, 2017

Updated @Makman2 . I hope this looks much cleaner and handles all cases now 😄

@@ -1,3 +1,6 @@
import requests
from shutil import which

This comment has been minimized.

@gitmate-bot

gitmate-bot Aug 10, 2017
Collaborator

This file contains unused source code.

PyUnusedCodeBear, severity NORMAL, section flakes.

The issue can be fixed by applying the following patch:

--- a/bears/yaml/TravisLintBear.py
+++ b/bears/yaml/TravisLintBear.py
@@ -1,5 +1,4 @@
 import requests
-from shutil import which
 
 from coalib.bearlib.abstractions.Linter import linter
 
@yash-nisar yash-nisar force-pushed the yash-nisar:travis-bug branch from 1b6ec14 to 18b71d2 Aug 10, 2017
@yash-nisar yash-nisar changed the title TravisLintBear: Check for internet connection WIP TravisLintBear: Check for internet connection Aug 10, 2017
@yash-nisar yash-nisar force-pushed the yash-nisar:travis-bug branch from 18b71d2 to c46d80b Aug 10, 2017
@yash-nisar yash-nisar force-pushed the yash-nisar:travis-bug branch from 53b1355 to 5c4e34f Aug 13, 2017
@classmethod
def check_prerequisites(cls):
base_check = super().check_prerequisites()
if base_check is not True: # pragma: no cover

This comment has been minimized.

@Makman2

Makman2 Aug 13, 2017
Member

can't you modify the environment so the check fails? Then you don't need the pragma

This comment has been minimized.

@Makman2

Makman2 Aug 13, 2017
Member

or you could patch super().check_prerequisites()

@yash-nisar yash-nisar force-pushed the yash-nisar:travis-bug branch from 5c4e34f to 1c771cc Aug 14, 2017
@yash-nisar
Copy link
Member Author

@yash-nisar yash-nisar commented Aug 14, 2017

@Makman2 I've removed the pragma: no cover and tested that snippet as well, but there is a coverage failure.

url_status.raise_for_status()
return True
except requests.exceptions.HTTPError:
return 'Failed to establish a connection to the server.'

This comment has been minimized.

@Makman2

Makman2 Aug 14, 2017
Member

"which server?" I would ask myself now if I get the message :) You maybe want to include the url to travis: CHECK_CONNECTION_URL

This comment has been minimized.

@yash-nisar

yash-nisar Aug 15, 2017
Author Member

Done 👍

@yash-nisar yash-nisar force-pushed the yash-nisar:travis-bug branch 3 times, most recently from fb2ed59 to 5e68799 Aug 15, 2017
@yash-nisar
Copy link
Member Author

@yash-nisar yash-nisar commented Aug 15, 2017

@Makman2 Should be ready now 😄

with requests_mock.Mocker() as m:
m.head(TravisLintBear.CHECK_CONNECTION_URL,
status_code=200)
self.assertTrue(TravisLintBear.check_prerequisites())

This comment has been minimized.

@Makman2

Makman2 Aug 15, 2017
Member

wanna also do assertEqual to avoid that failure-strings pass the test?

This comment has been minimized.

@yash-nisar

yash-nisar Aug 15, 2017
Author Member

Heh, the irony 😛

with patch.object(TravisLintBear.__bases__[1],
'check_prerequisites') as mock_method:
mock_method.return_value = 'travis is not installed.'
self.assertEqual('travis is not installed.',

This comment has been minimized.

@Makman2

Makman2 Aug 15, 2017
Member

maybe assign travis is not installed to a variable? up to you

@@ -24,6 +26,31 @@ class TravisLintBear:
LICENSE = 'AGPL-3.0'
CAN_DETECT = {'Formatting', 'Syntax'}
SEE_MORE = 'https://docs.travis-ci.com/user/travis-lint'
CHECK_CONNECTION_URL = 'https://travis-ci.org/'

This comment has been minimized.

@Makman2

Makman2 Aug 15, 2017
Member

maybe inline this into the check_prerequisites function? I think you won't use it anywhere else + now it looks like some bear-specific-special variable, but it isn't :)

@yash-nisar yash-nisar force-pushed the yash-nisar:travis-bug branch from 5e68799 to 80c01ba Aug 15, 2017
@yash-nisar
Copy link
Member Author

@yash-nisar yash-nisar commented Aug 15, 2017

Updated @Makman2

def check_prerequisites(cls):
base_check = super().check_prerequisites()
if base_check is not True:
return base_check

This comment has been minimized.

@Makman2

Makman2 Aug 15, 2017
Member

could you add a newline here? splits up the base check and the new check implemented by you :) I think it's really better to read with a newline here 👍

@yash-nisar yash-nisar force-pushed the yash-nisar:travis-bug branch from 80c01ba to 76b147e Aug 16, 2017
@yash-nisar
Copy link
Member Author

@yash-nisar yash-nisar commented Aug 16, 2017

Done @Makman2

@Makman2
Copy link
Member

@Makman2 Makman2 commented Aug 16, 2017

ack 76b147e

Add a check for internet connection in the `check_prerequisites`
method without which the bear will fail to run.

Fixes #1978
@yash-nisar yash-nisar force-pushed the yash-nisar:travis-bug branch from 76b147e to f51e54d Aug 17, 2017
@yash-nisar
Copy link
Member Author

@yash-nisar yash-nisar commented Aug 17, 2017

reack f51e54d

@yash-nisar
Copy link
Member Author

@yash-nisar yash-nisar commented Aug 17, 2017

@Makman2 Rebased and pushed 👍

@Makman2
Copy link
Member

@Makman2 Makman2 commented Aug 17, 2017

@rultor merge

@rultor
Copy link

@rultor rultor commented Aug 17, 2017

@rultor merge

@Makman2 OK, I'll try to merge now. You can check the progress of the merge here

@rultor rultor merged commit f51e54d into coala:master Aug 17, 2017
6 of 7 checks passed
6 of 7 checks passed
ci/circleci CircleCI is running your tests
Details
codecov/project 100% (target 100%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
review/gitmate/commit No issues with this one - go ahead! :)
Details
review/gitmate/manual This commit was acknowledged.
Details
review/gitmate/pr All is well! :) (0 problems solved)
Details
@rultor
Copy link

@rultor rultor commented Aug 17, 2017

@rultor merge

@Makman2 Done! FYI, the full log is here (took me 2min)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.