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.

Sign up for GitHub

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

updated reqs and test #447

Merged
merged 9 commits into from
Oct 11, 2019
Merged

updated reqs and test #447

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
828fdd4
updated reqs and test
unclebconnor Sep 25, 2019
8594abe
typo
unclebconnor Sep 25, 2019
c2bf9b7
added deprecation messages, unbroke change
unclebconnor Oct 2, 2019
e711706
updated asserts in test
unclebconnor Oct 4, 2019
d6a4525
updated tests and fixed linting errors
unclebconnor Oct 4, 2019
5ccf944
re-added handling for query_opt and type_opt
unclebconnor Oct 7, 2019
c71931c
updated printing and related tests
unclebconnor Oct 9, 2019
953561b
flake8 test and assert type
unclebconnor Oct 10, 2019
34fdf1e
added line for linter
unclebconnor Oct 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 38 additions & 8 deletions datadog/dogshell/monitor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
# datadog
from datadog import api
from datadog.dogshell.common import report_errors, report_warnings
import warnings
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove this import which is now unused and add back the blank line that's making flake8 tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated



class MonitorClient(object):
Expand Down Expand Up @@ -39,10 +40,24 @@ def setup_parser(cls, subparsers):

update_parser = verb_parsers.add_parser('update', help="Update existing monitor")
update_parser.add_argument('monitor_id', help="monitor to replace with the new definition")
update_parser.add_argument('type', help="type of the monitor, e.g. "
"'metric alert' 'service check'")
update_parser.add_argument('query', help="query to notify on with syntax varying"
" depending on what type of monitor you are creating")
update_parser.add_argument(
'type',
nargs='?',
help="[Deprecated] optional argument preferred"
"type of the monitor, e.g. 'metric alert' 'service check'",
default=None
)
update_parser.add_argument(
'query',
nargs='?',
help="[Deprecated] optional argument preferred"
"query to notify on with syntax varying depending on monitor type",
default=None
)
update_parser.add_argument('--type', help="type of the monitor, e.g. "
"'metric alert' 'service check'", default=None)
update_parser.add_argument('--query', help="query to notify on with syntax varying"
Copy link
Contributor

@zippolyte zippolyte Sep 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Unfortunately, changing the type of the parameters from positional args to named args is a breaking change. Can we just add the defaults without changing the name ? I guess not, we have to make them named arguments if we want to make them optional... Not sure how best to deal with that honestly

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a possible way forward is to:

  • Add nargs="?" to both type and query to make them optional.
  • Emit a deprecation warning if any of these two is set.
  • Add --type and --query optional arguments as well.
  • Raise an error if someone specifies both --type and the value for positional type. To make sure people use one or the other, but not both at the same time.

Does that make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. This solution seems ideal to me, thanks for the suggestions !
Do you think you can work on that @unclebconnor ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok sounds good. Happy to work on this. Thanks for the eyes!

Copy link
Contributor Author

@unclebconnor unclebconnor Oct 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

These updates are made. Working on tests next.

" depending on monitor type", default=None)
update_parser.add_argument('--name', help="name of the alert", default=None)
update_parser.add_argument('--message', help="message to include with "
"notifications for this monitor", default=None)
Expand Down Expand Up @@ -148,14 +163,29 @@ def _file_post(cls, args):
def _update(cls, args):
api._timeout = args.timeout
format = args.format
options = None

to_update = {}
if args.type:
to_update['type'] = args.type
msg = "[DEPRECATION] `type` is no longer required to `update` and may be omitted"
warnings.warn(msg, DeprecationWarning)
if args.query:
to_update['query'] = args.query
msg = "[DEPRECATION] `query` is no longer required to `update` and may be omitted"
warnings.warn(msg, DeprecationWarning)
Copy link
Contributor

Choose a reason for hiding this comment

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

Where do you use the new optional type and query arguments ? This is only for the deprecated positional ones. I think the parser will assign to the same variables, which is why your test works I assume.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah rats. It looks like I lost my updates here when I merged w/master and didn't notice. Will go back and fix. Used a different dest for the optional ones

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zippolyte added this back and tests passed locally. I didn't add anything to explicitly test this but there are tests that either positional or optional arguments and they're both passing

if args.name:
to_update['name'] = args.name
if args.message:
to_update['message'] = args.message

if args.options is not None:
try:
options = json.loads(args.options)
to_update['options'] = json.loads(args.options)
except:
raise Exception('bad json parameter')
res = api.Monitor.update(args.monitor_id, type=args.type, query=args.query,
name=args.name, message=args.message, options=options)

res = api.Monitor.update(args.monitor_id, **to_update)

report_warnings(res)
report_errors(res)
if format == 'pretty':
Expand Down
30 changes: 28 additions & 2 deletions tests/integration/dogshell/test_dogshell.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,7 @@ def test_monitors(self):
assert out["query"] == query
assert out["type"] == type_alert
monitor_id = str(out["id"])
monitor_name = out["name"]

out, _, _ = self.dogshell(["monitor", "show", monitor_id])
out = json.loads(out)
Expand All @@ -304,16 +305,41 @@ def test_monitors(self):

# Update options
options = {"notify_no_data": True, "no_data_timeframe": 20}

out, _, _ = self.dogshell(
["monitor", "update", monitor_id, type_alert, query, "--options", json.dumps(options)]
)

out = json.loads(out)
assert out["query"] == query
assert query in out['query']
assert out["options"]["notify_no_data"] == options["notify_no_data"]
assert out["options"]["no_data_timeframe"] == options["no_data_timeframe"]

# Update message only
updated_message = "monitor updated"
current_options = out['options']
out, err, return_code = self.dogshell(
["monitor", "update", monitor_id, "--message", updated_message]
)

out = json.loads(out)
assert updated_message == out["message"]
assert query == out['query']
assert monitor_name == out ['name']
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assert monitor_name == out ['name']
assert monitor_name == out['name']

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops! updated

assert current_options == out['options']

# Updating query only
updated_query = "avg(last_15m):sum:system.net.bytes_rcvd{*} by {env} > 222"

out, err, return_code = self.dogshell(
["monitor", "update", monitor_id, "--query", updated_query]
)

out = json.loads(out)
assert updated_query in out["query"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's also add an assert on the updated type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops yep sorry I overlooked this

assert updated_message in out['message'] # updated_message updated in previous step
assert monitor_name in out ['name']
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assert monitor_name in out ['name']
assert monitor_name in out['name']

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

assert current_options == out['options']

# Mute monitor
out, _, _ = self.dogshell(["monitor", "mute", str(out["id"])])
out = json.loads(out)
Expand Down