updated reqs and test #447

unclebconnor · 2019-09-25T21:29:18Z

The api.Monitor.update endpoint doesn't actually require query or type so removing restrictions from the clients and updating the docs (based on a customer request).

I updated the test in test_dogshell.py but wasn't able to get the test to run based on directions. Happy to test more thoroughly but could use some guidance.

zippolyte

Regarding the dog shell tests, we can trigger them manually in the CI. Otherwise, you'd need to provide the credentials of an org that can be used for testing only (tests are destructive, muting all monitors and such) and running tox -e integration.
Though a few tests are currently broken, I'm working on fixing them at the moment. So as long as you test manually that one command update command, we'll be good to merge.

zippolyte · 2019-09-26T07:57:42Z

datadog/dogshell/monitor.py

-                                   " depending on what type of monitor you are creating")
+        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"


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

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?

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

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

These updates are made. Working on tests next.

zippolyte · 2019-09-26T07:59:27Z

tests/integration/dogshell/test_dogshell.py

+            ["monitor", "update", "--message", updated_message])
+
+        assert "id" in out, out
+        assert "message" in out, out


let's remove the , out part here. It will prevent pytest from doing introspection.

zippolyte · 2019-09-26T08:00:10Z

tests/integration/dogshell/test_dogshell.py

+        self.assertEquals(out["message"], updated_message)
+        #confirming no other changes but message
+        self.assertEquals(out['query'], query)
+        self.assertEquals(out['type_alert'], type_alert)


let's use plain assert like above.

unclebconnor · 2019-10-03T20:11:13Z

Regarding the dog shell tests, we can trigger them manually in the CI. Otherwise, you'd need to provide the credentials of an org that can be used for testing only (tests are destructive, muting all monitors and such) and running tox -e integration.
Though a few tests are currently broken, I'm working on fixing them at the moment. So as long as you test manually that one command update command, we'll be good to merge.

@zippolyte I'm getting the following when running the tox command. Reading about this error online I'm seeing that it has to do with tox config. Is this known on our end at all? Happy to research more if not

COMP10192:datadogpy brian.connor$ tox -e integration
ERROR: unknown environment 'integration'

zippolyte · 2019-10-04T09:16:40Z

@unclebconnor
Yeah make sure you are based on the latest version of master. There has been a few changes made to the test suite for integration tests. On master the integration environment is there: https://github.com/DataDog/datadogpy/blob/master/tox.ini#L7
Then in order to run just the test you want, use tox -e integration-admin -- -k test_monitors (the monitors tests mutes and unmutes everything, that's why it's marked as admin, so that you don't accidentally run them with the rest of the suite)

You'll also have to address my comment about self.assertEquals then, because it will just fail now since we're not using unittest anymore.

unclebconnor · 2019-10-04T20:42:29Z

Ok I got a hand with this and I think it should be good to go. Waiting for some tests to pass but test_monitors works locally at least

zippolyte · 2019-10-07T07:57:59Z

/azp run Datadog.datadogpy.integration

azure-pipelines · 2019-10-07T07:58:09Z

Azure Pipelines successfully started running 1 pipeline(s).

zippolyte · 2019-10-07T08:19:49Z

datadog/dogshell/monitor.py

+        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)


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.

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

@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

zippolyte

Nice we're almost there !
I have a few more comments regarding how we print the warnings, and there seem to be syntax issues in your tests.
Also, let's test for the type update. It can be done at the same time as the query, since I guess when you change the type your query probably changes as well.

zippolyte · 2019-10-08T08:37:46Z

datadog/dogshell/monitor.py

+        if args.type:
+            if args.type_opt:
+                msg = '[WARNING] Duplicate arguments for `type`. Using optional value --type'
+                print(msg)


For printing, let's use the print_err utility function that you can import from datadog.dogshell.common

datadogpy/datadog/dogshell/common.py

Lines 11 to 16 in 9a463de

def print_err(msg):

if is_p3k():

print(msg + '\n', file=sys.stderr)

else:

sys.stderr.write(msg + '\n')

, and use the format "WARNING: " for consistency with how we print other warnings, as in

datadogpy/datadog/dogshell/common.py

Line 35 in 9a463de

print_err("WARNING: {}".format(warning))

This is all done and tests adjusted accordingly

zippolyte · 2019-10-08T08:43:36Z

tests/integration/dogshell/test_dogshell.py

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


Suggested change

assert monitor_name == out ['name']

assert monitor_name == out['name']

oops! updated

zippolyte · 2019-10-08T08:43:44Z

tests/integration/dogshell/test_dogshell.py

+        out = json.loads(out)
+        assert updated_query in out["query"]
+        assert updated_message in out['message'] # updated_message updated in previous step
+        assert monitor_name in out ['name']


Suggested change

assert monitor_name in out ['name']

assert monitor_name in out['name']

zippolyte · 2019-10-08T08:48:37Z

/azp run Datadog.datadogpy.integration

azure-pipelines · 2019-10-08T08:48:46Z

Azure Pipelines successfully started running 1 pipeline(s).

zippolyte

Should be good to go after those two additional changes 🙂

zippolyte · 2019-10-10T09:13:48Z

datadog/dogshell/monitor.py

@@ -7,10 +7,9 @@

 # datadog
 from datadog import api
-from datadog.dogshell.common import report_errors, report_warnings
+from datadog.dogshell.common import report_errors, report_warnings, print_err
 import warnings


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

zippolyte · 2019-10-10T09:15:08Z

tests/integration/dogshell/test_dogshell.py


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

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


Let's also add an assert on the updated type

oops yep sorry I overlooked this

unclebconnor · 2019-10-10T21:22:34Z

Should be good to go after those two additional changes 🙂

Thanks for your patience stepping through the process here!

zippolyte · 2019-10-11T08:15:38Z

/azp run DataDog.datadogpy.integration

azure-pipelines · 2019-10-11T08:15:48Z

Azure Pipelines successfully started running 1 pipeline(s).

* updated reqs and test * typo * added deprecation messages, unbroke change * updated asserts in test * updated tests and fixed linting errors * re-added handling for query_opt and type_opt * updated printing and related tests * flake8 test and assert type * added line for linter

unclebconnor requested a review from a team as a code owner September 25, 2019 21:29

zippolyte requested changes Sep 26, 2019

View reviewed changes

unclebconnor added 4 commits October 4, 2019 12:16

updated reqs and test

828fdd4

typo

8594abe

added deprecation messages, unbroke change

c2bf9b7

updated asserts in test

e711706

unclebconnor force-pushed the bcon/monitor-update-non-required-fields branch from 66a24ab to e711706 Compare October 4, 2019 20:35

updated tests and fixed linting errors

d6a4525

zippolyte requested changes Oct 7, 2019

View reviewed changes

re-added handling for query_opt and type_opt

5ccf944

zippolyte requested changes Oct 8, 2019

View reviewed changes

updated printing and related tests

c71931c

zippolyte requested changes Oct 10, 2019

View reviewed changes

flake8 test and assert type

953561b

unclebconnor mentioned this pull request Oct 11, 2019

Bcon/make query optional monitor update DataDog/dogapi-rb#192

Merged

added line for linter

34fdf1e

zippolyte approved these changes Oct 11, 2019

View reviewed changes

unclebconnor merged commit ac14bda into DataDog:master Oct 11, 2019

unclebconnor deleted the bcon/monitor-update-non-required-fields branch October 11, 2019 16:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

updated reqs and test #447

updated reqs and test #447

unclebconnor commented Sep 25, 2019 •

edited

Loading

zippolyte left a comment

zippolyte Sep 26, 2019 •

edited

Loading

bkabrda Sep 26, 2019

zippolyte Sep 26, 2019

unclebconnor Sep 26, 2019

unclebconnor Oct 3, 2019 •

edited

Loading

zippolyte Sep 26, 2019

zippolyte Sep 26, 2019

unclebconnor commented Oct 3, 2019 •

edited

Loading

zippolyte commented Oct 4, 2019 •

edited

Loading

unclebconnor commented Oct 4, 2019 •

edited

Loading

zippolyte commented Oct 7, 2019

azure-pipelines bot commented Oct 7, 2019

zippolyte Oct 7, 2019

unclebconnor Oct 7, 2019

unclebconnor Oct 7, 2019

zippolyte left a comment

zippolyte Oct 8, 2019

unclebconnor Oct 9, 2019

zippolyte Oct 8, 2019

unclebconnor Oct 9, 2019

zippolyte Oct 8, 2019

unclebconnor Oct 9, 2019

zippolyte commented Oct 8, 2019

azure-pipelines bot commented Oct 8, 2019

zippolyte left a comment

zippolyte Oct 10, 2019

unclebconnor Oct 10, 2019

zippolyte Oct 10, 2019

unclebconnor Oct 10, 2019

unclebconnor commented Oct 10, 2019

zippolyte commented Oct 11, 2019

azure-pipelines bot commented Oct 11, 2019

	def print_err(msg):
	if is_p3k():
	print(msg + '\n', file=sys.stderr)
	else:
	sys.stderr.write(msg + '\n')

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

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

updated reqs and test #447

updated reqs and test #447

Conversation

unclebconnor commented Sep 25, 2019 • edited Loading

zippolyte left a comment

Choose a reason for hiding this comment

zippolyte Sep 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unclebconnor Oct 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unclebconnor commented Oct 3, 2019 • edited Loading

zippolyte commented Oct 4, 2019 • edited Loading

unclebconnor commented Oct 4, 2019 • edited Loading

zippolyte commented Oct 7, 2019

azure-pipelines bot commented Oct 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zippolyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zippolyte commented Oct 8, 2019

azure-pipelines bot commented Oct 8, 2019

zippolyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unclebconnor commented Oct 10, 2019

zippolyte commented Oct 11, 2019

azure-pipelines bot commented Oct 11, 2019

unclebconnor commented Sep 25, 2019 •

edited

Loading

zippolyte Sep 26, 2019 •

edited

Loading

unclebconnor Oct 3, 2019 •

edited

Loading

unclebconnor commented Oct 3, 2019 •

edited

Loading

zippolyte commented Oct 4, 2019 •

edited

Loading

unclebconnor commented Oct 4, 2019 •

edited

Loading