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

Make connectivity check domains configurable #12571

Merged
merged 1 commit into from Nov 22, 2018
Merged

Make connectivity check domains configurable #12571

merged 1 commit into from Nov 22, 2018

Conversation

@TheLastProject
Copy link
Contributor

@TheLastProject TheLastProject commented Nov 21, 2018

Fixes #12370.

I've made sure to keep the default to the same domains. This allows you to keep has_internet_connection to true, so that you can still install apps and the likes, but not ping to domains you don't want to ping to.

If the domain list in 'connectivity_check_domains' is explicitly empty, Nextcloud will still show the connectivity warning, as there are no hosts to match.

lib/private/Setup.php Outdated Show resolved Hide resolved
@kesselb
Copy link
Contributor

@kesselb kesselb commented Nov 21, 2018

@kesselb kesselb added this to the Nextcloud 16 milestone Nov 21, 2018
@TheLastProject
Copy link
Contributor Author

@TheLastProject TheLastProject commented Nov 21, 2018

I have made the requested changes

Copy link
Member

@rullzer rullzer left a comment

Cool stuff!

Copy link
Contributor

@kesselb kesselb left a comment

Works for me

@kesselb
Copy link
Contributor

@kesselb kesselb commented Nov 21, 2018

First-time contributor

Thank you @TheLastProject for your contribution 🎉

@TheLastProject
Copy link
Contributor Author

@TheLastProject TheLastProject commented Nov 21, 2018

First-time contributor

Thank you @TheLastProject for your contribution tada

Thank you for helping me through making a good pull request :)

@MorrisJobke
Copy link
Member

@MorrisJobke MorrisJobke commented Nov 22, 2018

@TheLastProject Thanks for the contribution.

There are 3 unit tests failing due to the new check of the config:

There were 3 failures:

1) Tests\Settings\Controller\CheckSetupControllerTest::testIsInternetConnectionWorkingCorrectly
OCP\IConfig::getSystemValue('connectivity_check_domains', Array (...)) was not expected to be called more than once.

/drone/src/github.com/nextcloud/server/settings/Controller/CheckSetupController.php:134
/drone/src/github.com/nextcloud/server/tests/lib/TestCase.php:210
/drone/src/github.com/nextcloud/server/tests/Settings/Controller/CheckSetupControllerTest.php:210

2) Tests\Settings\Controller\CheckSetupControllerTest::testIsInternetConnectionFail
OCP\IConfig::getSystemValue('connectivity_check_domains', Array (...)) was not expected to be called more than once.

/drone/src/github.com/nextcloud/server/settings/Controller/CheckSetupController.php:134
/drone/src/github.com/nextcloud/server/tests/lib/TestCase.php:210
/drone/src/github.com/nextcloud/server/tests/Settings/Controller/CheckSetupControllerTest.php:234

3) Tests\Settings\Controller\CheckSetupControllerTest::testCheck
Expectation failed for method name is equal to "getSystemValue" when invoked at sequence index 2
Parameter 0 for invocation OCP\IConfig::getSystemValue('connectivity_check_domains', Array (...)) does not match expected value.
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'memcache.local'
+'connectivity_check_domains'

/drone/src/github.com/nextcloud/server/settings/Controller/CheckSetupController.php:134
/drone/src/github.com/nextcloud/server/settings/Controller/CheckSetupController.php:600
/drone/src/github.com/nextcloud/server/tests/Settings/Controller/CheckSetupControllerTest.php:520

You can run the unit tests locally via following command:

./autotest.sh sqlite tests/Settings/Controller/CheckSetupControllerTest.php

If you don't specify the path, then all are executed and this could take very long. Often I also clear my instance and completely reset it (otherwise the currently installed data in the DB and data directory or the config options could manipulate the test run) via rm -rf data/* config/config.php. Just to not let you run into such problems. Could you look into fixing them? If you need further help, don't hesitate to ask questions here.

@nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Nov 22, 2018

You only modified the working test, the negative tests also need adjusting ;)

There were 2 failures:

  1. Tests\Settings\Controller\CheckSetupControllerTest::testIsInternetConnectionWorkingCorrectly
    OCP\IConfig::getSystemValue('connectivity_check_domains', Array (...)) was not expected to be called more than once.

/drone/src/github.com/nextcloud/server/settings/Controller/CheckSetupController.php:134
/drone/src/github.com/nextcloud/server/tests/lib/TestCase.php:210
/drone/src/github.com/nextcloud/server/tests/Settings/Controller/CheckSetupControllerTest.php:210

  1. Tests\Settings\Controller\CheckSetupControllerTest::testIsInternetConnectionFail
    OCP\IConfig::getSystemValue('connectivity_check_domains', Array (...)) was not expected to be called more than once.

/drone/src/github.com/nextcloud/server/settings/Controller/CheckSetupController.php:134
/drone/src/github.com/nextcloud/server/tests/lib/TestCase.php:210
/drone/src/github.com/nextcloud/server/tests/Settings/Controller/CheckSetupControllerTest.php:234

@TheLastProject
Copy link
Contributor Author

@TheLastProject TheLastProject commented Nov 22, 2018

Was having some issues getting the tests to run locally, so wanted to see if I could get one of them fixed at least. Now I know what to do properly, yeah, thanks :)

Signed-off-by: Sylvia van Os <sylvia@hackerchick.me>
@TheLastProject
Copy link
Contributor Author

@TheLastProject TheLastProject commented Nov 22, 2018

Tests seem to all pass now again :)

@MorrisJobke
Copy link
Member

@MorrisJobke MorrisJobke commented Nov 22, 2018

Tests seem to all pass now again :)

Looks good 👍

@MorrisJobke
Copy link
Member

@MorrisJobke MorrisJobke commented Nov 22, 2018

@nickvergessen @rullzer As this is a minor fix that would solve some issues for people should we merge this into 15? What do you think? I would be fine, because the actual code is very minimal (basically a config fetch).

@nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Nov 22, 2018

FIne by me

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Code makes sense 👍

@MorrisJobke MorrisJobke merged commit db6fad3 into nextcloud:master Nov 22, 2018
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/drone/pr the build failed
Details
@dco
DCO DCO
Details
@fixupbot
fixupbot No fixup commits found. The commit history is clean
Details
@welcome
Copy link

@welcome welcome bot commented Nov 22, 2018

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22
Most developers hang out on IRC. So join #nextcloud-dev on Freenode for a chat!

@TheLastProject TheLastProject deleted the TheLastProject:feature/make_connectivity_check_domains_changeable branch Nov 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants