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 support for dcos-log v2 #1105
Conversation
run linux integration tests |
09af8d6
to
adc7225
Compare
adc7225
to
bbe72fe
Compare
run linux integration tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments, mostly typos
cli/dcoscli/log.py
Outdated
@@ -221,7 +221,19 @@ def has_journald_capability(): | |||
get_cosmos_url()).has_capability('LOGGING') | |||
|
|||
|
|||
def dcos_log_enabled(): | |||
def has_log_v2_capability(): | |||
""" function checks thec cosmos capability LOGGING_V2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thec
-> the
cli/dcoscli/node/main.py
Outdated
:return: leader logs url | ||
:rtype: str | ||
""" | ||
|
||
if version < 1 and version > 2: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can't be evaluated to true, did you mean or
?
cli/dcoscli/task/main.py
Outdated
|
||
if len(tasks) != 1: | ||
raise DCOSException( | ||
"found more then one task with the same name: {}. Please provide " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then
-> than
cli/dcoscli/task/main.py
Outdated
:param follow: same as unix tail's -f | ||
:type follow: bool | ||
:param task: task pattern to match | ||
:type task: str |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the param name is tasks
cli/tests/unit/test_node.py
Outdated
@mock.patch('dcos.config.get_config_val') | ||
@mock.patch('dcos.http.get') | ||
@mock.patch('dcoscli.log.dcos_log_enabled') | ||
def test_dcos_log_v2_leader_mesos(mocked_dcos_log_enabked, mocked_http_get, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are multiple occurences of mocked_dcos_log_enabked
instead of mocked_dcos_log_enabled
in this file.
dcos task <name> log will check the LOGGING_V2 capability first. If V2 is enabled, it will be used by default. Otherwise it will check for logging strategy in the cluster and use either mesos files API directly or dcos-log v1.
7b4d2f4
to
3c30a40
Compare
run linux integration tests |
@bamarni i addressed your comments, please review again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
affected subcommands:
dcos task <name> log
dcos node log ...
dcos service log <name>
If DC/OS cluster has the
LOGGING_V2
capability, dcos-cli should use dcos-log v2, otherwise based on the logging strategy mesos files API or dcos-log v1 will be used.