[GitHub] [mesos] andreaspeters commented on a change in pull request #383: Add mesos authentication to the mesos cli
andreaspeters commented on a change in pull request #383: URL: https://github.com/apache/mesos/pull/383#discussion_r632179279 ## File path: src/python/cli_new/lib/cli/mesos.py ## @@ -199,11 +199,14 @@ def __init__(self, master, task_id): "This command is only supported for tasks" " launched by the Universal Container Runtime (UCR).") +# Get the scheme of the agent Review comment: Oh yes, that's pretty. Didn't know that. :-) Thanks @cf-natali -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #383: Add mesos authentication to the mesos cli
andreaspeters commented on a change in pull request #383: URL: https://github.com/apache/mesos/pull/383#discussion_r632178496 ## File path: src/python/cli_new/lib/cli/http.py ## @@ -19,70 +19,71 @@ """ import json -import urllib.request -import urllib.error -import urllib.parse -import time +from urllib.parse import urlencode +import urllib3 import cli from cli.exceptions import CLIException +urllib3.disable_warnings() -def read_endpoint(addr, endpoint, query=None): +def read_endpoint(addr, endpoint, config, query=None): """ Read the specified endpoint and return the results. """ + +data = None Review comment: yes, u are right. :+1: -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #383: Add mesos authentication to the mesos cli
andreaspeters commented on a change in pull request #383: URL: https://github.com/apache/mesos/pull/383#discussion_r632177344 ## File path: src/python/cli_new/lib/cli/mesos.py ## @@ -114,15 +113,16 @@ def get_container_id(task): " Please try again.") -def get_tasks(master, query=None): +# pylint: disable=dangerous-default-value Review comment: done -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #383: Add mesos authentication to the mesos cli
andreaspeters commented on a change in pull request #383:
URL: https://github.com/apache/mesos/pull/383#discussion_r632176827
##
File path: src/python/cli_new/lib/cli/http.py
##
@@ -19,70 +19,71 @@
"""
import json
-import urllib.request
-import urllib.error
-import urllib.parse
-import time
+from urllib.parse import urlencode
+import urllib3
import cli
from cli.exceptions import CLIException
+urllib3.disable_warnings()
-def read_endpoint(addr, endpoint, query=None):
+def read_endpoint(addr, endpoint, config, query=None):
"""
Read the specified endpoint and return the results.
"""
+
+data = None
try:
addr = cli.util.sanitize_address(addr)
except Exception as exception:
raise CLIException("Unable to sanitize address '{addr}': {error}"
.format(addr=addr, error=str(exception)))
-
try:
url = "{addr}/{endpoint}".format(addr=addr, endpoint=endpoint)
if query is not None:
-url += "?{query}".format(query=urllib.parse.urlencode(query))
-http_response = urllib.request.urlopen(url).read().decode("utf-8")
+url += "?{query}".format(query=urlencode(query))
+if config.principal() is not None and config.secret() is not None:
+headers = urllib3.make_headers(
+basic_auth=config.principal() + ":" + config.secret()
+)
+else:
+headers = None
+http = urllib3.PoolManager()
+http_response = http.request('GET', url, headers=headers, timeout=5)
+data = http_response.data.decode('utf-8')
+
except Exception as exception:
raise CLIException("Unable to open url '{url}': {error}"
.format(url=url, error=str(exception)))
-return http_response
+return data
-def get_json(addr, endpoint, condition=None, timeout=5, query=None):
+def get_json(addr, endpoint, config, condition=None, query=None):
"""
Return the contents of the 'endpoint' at 'addr' as JSON data
subject to the condition specified in 'condition'. If we are
-unable to read the data or unable to meet the condition within
-'timeout' seconds we throw an error.
+unable to read the data we throw an error.
"""
-start_time = time.time()
-
-while True:
-data = None
-try:
-data = read_endpoint(addr, endpoint, query)
-except Exception as exception:
-pass
+data = None
-if data:
-try:
-data = json.loads(data)
-except Exception as exception:
-raise CLIException("Could not load JSON from '{data}': {error}"
- .format(data=data, error=str(exception)))
+try:
+data = read_endpoint(addr, endpoint, config, query)
+except Exception as exception:
+pass
Review comment:
Sorry this part I just removed the loop around. I don't know what the
idea behind this exception handling was. But I think u make a good point. Let
me have a look to see if I can change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [mesos] cf-natali commented on a change in pull request #383: Add mesos authentication to the mesos cli
cf-natali commented on a change in pull request #383:
URL: https://github.com/apache/mesos/pull/383#discussion_r632104102
##
File path: src/python/cli_new/lib/cli/http.py
##
@@ -19,70 +19,71 @@
"""
import json
-import urllib.request
-import urllib.error
-import urllib.parse
-import time
+from urllib.parse import urlencode
+import urllib3
import cli
from cli.exceptions import CLIException
+urllib3.disable_warnings()
-def read_endpoint(addr, endpoint, query=None):
+def read_endpoint(addr, endpoint, config, query=None):
"""
Read the specified endpoint and return the results.
"""
+
+data = None
Review comment:
Looking at the code below it doesn't seem that the function can ever
return `None` for `data` - maybe no need to set it to `None` here, and just
directly return below:
```
return http_response.data.decode('utf-8')
```
##
File path: src/python/cli_new/lib/cli/http.py
##
@@ -19,70 +19,71 @@
"""
import json
-import urllib.request
-import urllib.error
-import urllib.parse
-import time
+from urllib.parse import urlencode
+import urllib3
import cli
from cli.exceptions import CLIException
+urllib3.disable_warnings()
-def read_endpoint(addr, endpoint, query=None):
+def read_endpoint(addr, endpoint, config, query=None):
"""
Read the specified endpoint and return the results.
"""
+
+data = None
try:
addr = cli.util.sanitize_address(addr)
except Exception as exception:
raise CLIException("Unable to sanitize address '{addr}': {error}"
.format(addr=addr, error=str(exception)))
-
try:
url = "{addr}/{endpoint}".format(addr=addr, endpoint=endpoint)
if query is not None:
-url += "?{query}".format(query=urllib.parse.urlencode(query))
-http_response = urllib.request.urlopen(url).read().decode("utf-8")
+url += "?{query}".format(query=urlencode(query))
+if config.principal() is not None and config.secret() is not None:
+headers = urllib3.make_headers(
+basic_auth=config.principal() + ":" + config.secret()
+)
+else:
+headers = None
+http = urllib3.PoolManager()
+http_response = http.request('GET', url, headers=headers, timeout=5)
+data = http_response.data.decode('utf-8')
+
except Exception as exception:
raise CLIException("Unable to open url '{url}': {error}"
.format(url=url, error=str(exception)))
-return http_response
+return data
-def get_json(addr, endpoint, condition=None, timeout=5, query=None):
+def get_json(addr, endpoint, config, condition=None, query=None):
"""
Return the contents of the 'endpoint' at 'addr' as JSON data
subject to the condition specified in 'condition'. If we are
-unable to read the data or unable to meet the condition within
-'timeout' seconds we throw an error.
+unable to read the data we throw an error.
"""
-start_time = time.time()
-
-while True:
-data = None
-try:
-data = read_endpoint(addr, endpoint, query)
-except Exception as exception:
-pass
+data = None
-if data:
-try:
-data = json.loads(data)
-except Exception as exception:
-raise CLIException("Could not load JSON from '{data}': {error}"
- .format(data=data, error=str(exception)))
+try:
+data = read_endpoint(addr, endpoint, config, query)
+except Exception as exception:
+pass
Review comment:
Hm, I'm not sure I understand what's going on here - the function's
docstring says "If we are unable to read the data we throw an error.", so why
catch the exception?
And also, if we catch an exception here, `data` will be `None`, so the below
`data = json.loads(data)` is definitely not going to work.
##
File path: src/python/cli_new/lib/cli/mesos.py
##
@@ -199,11 +199,14 @@ def __init__(self, master, task_id):
"This command is only supported for tasks"
" launched by the Universal Container Runtime (UCR).")
+# Get the scheme of the agent
Review comment:
Might be more readable:
```
scheme = "https://; if config.agent_ssl() else "http://;
```
##
File path: src/python/cli_new/lib/cli/config.py
##
@@ -119,6 +119,65 @@ def master(self):
return master
+def principal(self):
+"""
+Return the principal in the configuration file
+"""
+if "principal" not in self.data["master"]:
+return None
Review comment:
Expanding on @asekretenko suggestion, this might be better:
```
principal = self.data["master"].get("principal")
if principal is None:
return None
elif not
[GitHub] [mesos] cf-natali opened a new pull request #384: Fixed parsing of ld.so.cache on new glibc.
cf-natali opened a new pull request #384: URL: https://github.com/apache/mesos/pull/384 Since glibc 2.32, `ld.so.cache` now defaults to the "new" format, instead of the "compat" format which was in use since glibc 2.2 (around 20 years ago). The code now supports both the "compat" and "new" formats: Before: ``` root@thinkpad:/home/cf/src/mesos/build# ldconfig -c new root@thinkpad:/home/cf/src/mesos/build# ./bin/mesos-tests.sh --gtest_filter=*Ld* [...] [==] Running 4 tests from 2 test cases. [--] Global test environment set-up. [--] 1 test from LdcacheTest [ RUN ] LdcacheTest.Parse ../../src/tests/ldcache_tests.cpp:43: Failure cache: Invalid format [ FAILED ] LdcacheTest.Parse (0 ms) [--] 1 test from LdcacheTest (0 ms total) [--] 3 tests from Ldd [ RUN ] Ldd.BinSh ../../src/tests/ldd_tests.cpp:43: Failure cache: Invalid format [ FAILED ] Ldd.BinSh (0 ms) [ RUN ] Ldd.EmptyCache [ OK ] Ldd.EmptyCache (1 ms) [ RUN ] Ldd.MissingFile ../../src/tests/ldd_tests.cpp:77: Failure cache: Invalid format [ FAILED ] Ldd.MissingFile (0 ms) [--] 3 tests from Ldd (1 ms total) [--] Global test environment tear-down [==] 4 tests from 2 test cases ran. (8 ms total) [ PASSED ] 1 test. [ FAILED ] 3 tests, listed below: [ FAILED ] LdcacheTest.Parse [ FAILED ] Ldd.BinSh [ FAILED ] Ldd.MissingFile 3 FAILED TESTS ``` After: ``` root@thinkpad:/home/cf/src/mesos/build# ldconfig -c new root@thinkpad:/home/cf/src/mesos/build# ./bin/mesos-tests.sh --gtest_filter=*Ld* [...] [==] Running 4 tests from 2 test cases. [--] Global test environment set-up. [--] 1 test from LdcacheTest [ RUN ] LdcacheTest.Parse [ OK ] LdcacheTest.Parse (529 ms) [--] 1 test from LdcacheTest (529 ms total) [--] 3 tests from Ldd [ RUN ] Ldd.BinSh [ OK ] Ldd.BinSh (3 ms) [ RUN ] Ldd.EmptyCache [ OK ] Ldd.EmptyCache (0 ms) [ RUN ] Ldd.MissingFile [ OK ] Ldd.MissingFile (0 ms) [--] 3 tests from Ldd (3 ms total) [--] Global test environment tear-down [==] 4 tests from 2 test cases ran. (541 ms total) [ PASSED ] 4 tests. ``` Closes #10220. @asekretenko @andreaspeters -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
