Add command to create new integrations #2037

ofek · 2018-08-13T02:13:05Z

Additional Notes

In a subsequent PR we will add different types of integrations too.

gmmeyer · 2018-08-13T02:43:23Z

datadog_checks_dev/datadog_checks/dev/tooling/files/manifest.py

+
+from .utils import File
+
+TEMPLATE_IN = """\


can we put this (and the others) in separate files and read it in? I think it'll be easier to upkeep than as a string

jeremy-lq · 2018-08-13T02:49:53Z

@phrawzty, let's make sure this is included in the updated developer docs.

gzussa · 2018-08-13T08:50:32Z

datadog_checks_dev/datadog_checks/dev/tooling/files/tox.py

+[testenv]
+platform = linux|darwin|win32
+deps =
+    ../datadog_checks_base


gzussa · 2018-08-13T08:51:10Z

datadog_checks_dev/datadog_checks/dev/tooling/files/setup.py

+    ],
+
+    # The package we're going to ship
+    packages=['datadog_checks.{check_name}'],


'datadog_checks', 'datadog_checks.{check_name}' ?

gzussa

This is great 😃 and it will greatly help everyone 💯 but it needs to be updated. Here are my 3 main points:

First, I totally support @gmmeyer on using templates in separate files.
Also, some of these templates are not up to date (see inline comments).
Finally, we need a test that would generate a dummy check using this create a function and makes sure that the CI passes. It would be pretty bad if the generator didn't generate a valid skeleton.

gzussa · 2018-08-13T08:53:45Z

datadog_checks_dev/datadog_checks/dev/tooling/files/manifest.py

+
+TEMPLATE_JSON = """\
+{{
+  "display_name": "{check_name_cap}",


This template is not accurate. See doc. Some of these fields do not exist anymore

zippolyte · 2018-08-14T08:29:30Z

Question: why not make a call to cookiecutter and apply the template we have in https://github.com/DataDog/cookiecutter-datadog-check ?

phrawzty · 2018-08-14T09:11:37Z

call to cookiecutter and apply the template

This is currently (i.e. in our docs) the recommended way of generating the base files.

ofek · 2018-08-14T15:16:36Z

@zippolyte The main reason is we need a way to create different types of integrations e.g. tile-only, etc. and cookiecutter can’t do conditionals.

phrawzty · 2018-08-14T15:20:58Z

need a way to create different types of integrations

More than one cookiecutter template?

ofek · 2018-08-14T15:25:44Z

@phrawzty Yes exactly!

zippolyte · 2018-08-14T15:30:01Z

Yeah in that case we could have more than one cookiecutter template and do the conditionals in the dev package to choose the right one.
The issue I see with having the actual templates in the dev package is that it will make it more difficult to maintain the templates since you'd have to release the dev package when you make a change to them.

ofek · 2018-08-14T15:38:20Z

I think that's a fair tradeoff for having everything in one place. After all, a release is just one command.

Also, cookiecutter can't do dynamic things we need like normalizing package/check class names, creating a new GUID for the manifest, etc.

zippolyte

Looks great, just one minor comment.

zippolyte · 2018-08-17T09:37:47Z

datadog_checks_dev/datadog_checks/dev/tooling/commands/create.py

+@click.pass_context
+def create(ctx, name, integration_type, location, quiet, dry_run):
+    """Create a new integration."""
+    integration_name = normalize_package_name(name)


I think we should also keep a reference to name as is, and use it as the display name in README and so on, instead of check_name_cap

phrawzty · 2018-08-17T11:39:06Z

datadog_checks_dev/datadog_checks/dev/tooling/templates/check/{check_name}/README.md

@@ -0,0 +1,47 @@
+# Agent Check: {check_name_cap}


Please add a line break between these headers.

phrawzty · 2018-08-17T11:39:29Z

datadog_checks_dev/datadog_checks/dev/tooling/templates/check/{check_name}/README.md

+
+[Run the Agent's `status` subcommand][5] and look for `{check_name}` under the Checks section.
+
+## Data Collected


Line break please :)

phrawzty · 2018-08-17T11:41:54Z

datadog_checks_dev/datadog_checks/dev/tooling/templates/check/{check_name}/README.md

+## Data Collected
+### Metrics
+
+The {check_name_cap} check does not include any metrics at this time.


We don't need to say "at this time" here. The documentation represents now, not a hypothetical future.

The {check_name_cap} check does not include any metrics.

Imho, we could even shorten this to:

{check_name_cap} does not include any metrics.

phrawzty · 2018-08-17T11:42:05Z

datadog_checks_dev/datadog_checks/dev/tooling/templates/check/{check_name}/README.md

+
+### Service Checks
+
+The {check_name_cap} check does not include any service checks at this time.


phrawzty · 2018-08-17T11:42:10Z

datadog_checks_dev/datadog_checks/dev/tooling/templates/check/{check_name}/README.md

+
+### Events
+
+The {check_name_cap} check does not include any events at this time.


phrawzty · 2018-08-17T11:43:47Z

datadog_checks_dev/datadog_checks/dev/tooling/templates/check/{check_name}/README.md

+
+Need help? Contact [Datadog Support][6].
+
+[1]: link to integration's site


Should this be a templatable item as well? If that's problematic, we should surface this more obviously, like** LINK_TO_INTEGERATION_SITE ** or something 😄

phrawzty · 2018-08-17T11:44:09Z

datadog_checks_dev/datadog_checks/dev/tooling/templates/check/{check_name}/manifest.json

@@ -0,0 +1,23 @@
+{{


Just checking - is the double-nesting here correct?

It is, since we use curly braces for template variables, we need to escape the actual curly braces by doubling them

phrawzty · 2018-08-17T11:46:35Z

datadog_checks_dev/README.md

+$ ddev create -h
+Usage: ddev create [OPTIONS] NAME
+
+  Create a new integration.


This doesn't actually create a new Integration - it sets up the framework for a new Integration. We should be clear about that here and in the Options: below.

phrawzty · 2018-08-17T11:49:29Z

datadog_checks_dev/datadog_checks/dev/tooling/templates/check/{check_name}/README.md

+
+### Installation
+
+The {check_name_cap} check is included in the [Datadog Agent][2] package, so you don't


This is true of core Integrations, but not of extras; suggest hedging our default language here.

gmmeyer · 2018-08-17T18:55:56Z

datadog_checks_dev/datadog_checks/dev/tooling/templates/check/{check_name}/tests/__init__.py

@@ -0,0 +1,3 @@
+# (C) Datadog, Inc. {year}


Unless they're datadog employees we dont want to be forcing them to be signing over copyright to Datadog. In extras this could cause issues.

@jeremy-lq what should we do here? while this is useful for dd employees, it's not necessarily something we want to do for other authors in Extras.

We may want to move the license and copyright notice to a conditional based on repo_choice.

jeremy-lq · 2018-08-17T20:00:11Z

datadog_checks_dev/datadog_checks/dev/tooling/templates/check/{check_name}/setup.py

+    author_email='{email_packages}',
+
+    # License
+    license='BSD',


Let's change this to the SPDX identifier: BSD-3-Clause

zippolyte · 2018-08-20T11:17:01Z

What about my comment on using the original name parameter (before calling normalize_integration_name) as a display name (in README, CHANGELOG and manifest.json) ? Don't you think it would be better than using check_name_cap ?

ofek · 2018-08-20T15:13:33Z

@zippolyte I did that a262d7e#diff-14d3f79bfec6320bc25a1333c056fcafR112

zippolyte

LGTM, thanks for the changes ❤️

* Add command to create new integrations

ofek added changelog/Added integration/datadog_checks_dev labels Aug 13, 2018

ofek requested review from a team as code owners August 13, 2018 02:13

gmmeyer reviewed Aug 13, 2018

View reviewed changes

Add command to create new integrations

6743846

ofek force-pushed the ofek/create branch from f286539 to 6743846 Compare August 13, 2018 02:56

gzussa reviewed Aug 13, 2018

View reviewed changes

gzussa self-requested a review August 13, 2018 08:54

gzussa suggested changes Aug 13, 2018

View reviewed changes

ofek added 3 commits August 16, 2018 01:40

Address review

7a80cc1

Make tests ignore the templates

3f839d6

Add test

681372d

ofek force-pushed the ofek/create branch from b3284ae to 681372d Compare August 16, 2018 18:57

Format only our path name

d1d40e6

ofek force-pushed the ofek/create branch from be6014f to 7102ae7 Compare August 17, 2018 03:33

Update for #2067

8e2b936

ofek force-pushed the ofek/create branch from 7102ae7 to 8e2b936 Compare August 17, 2018 03:43

zippolyte requested changes Aug 17, 2018

View reviewed changes

phrawzty suggested changes Aug 17, 2018

View reviewed changes

address review

a262d7e

gmmeyer reviewed Aug 17, 2018

View reviewed changes

jeremy-lq reviewed Aug 17, 2018

View reviewed changes

ofek added 2 commits August 18, 2018 16:32

address review

5ec23d9

refactor

3ccdadc

ofek force-pushed the ofek/create branch from 1d4516e to 3ccdadc Compare August 19, 2018 01:35

phrawzty previously approved these changes Aug 20, 2018

View reviewed changes

ofek dismissed phrawzty’s stale review via 2f1ea8e August 20, 2018 17:32

ofek force-pushed the ofek/create branch from 2f1ea8e to 33f0856 Compare August 20, 2018 17:48

address review

c2eff60

ofek force-pushed the ofek/create branch from 33f0856 to c2eff60 Compare August 20, 2018 18:15

zippolyte approved these changes Aug 20, 2018

View reviewed changes

ofek merged commit a7d41ac into master Aug 20, 2018

ofek deleted the ofek/create branch August 20, 2018 21:42

nmuesch pushed a commit that referenced this pull request Nov 1, 2018

Add command to create new integrations (#2037)

0e5d87e

* Add command to create new integrations

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add command to create new integrations #2037

Add command to create new integrations #2037

ofek commented Aug 13, 2018 •

edited

Loading

gmmeyer Aug 13, 2018 •

edited

Loading

jeremy-lq commented Aug 13, 2018

gzussa Aug 13, 2018

gzussa Aug 13, 2018 •

edited

Loading

gzussa left a comment •

edited

Loading

gzussa Aug 13, 2018

zippolyte commented Aug 14, 2018

phrawzty commented Aug 14, 2018

ofek commented Aug 14, 2018

phrawzty commented Aug 14, 2018

ofek commented Aug 14, 2018

zippolyte commented Aug 14, 2018

ofek commented Aug 14, 2018

zippolyte left a comment

zippolyte Aug 17, 2018 •

edited

Loading

phrawzty Aug 17, 2018

ofek Aug 17, 2018

phrawzty Aug 17, 2018

ofek Aug 17, 2018

phrawzty Aug 17, 2018

phrawzty Aug 17, 2018

phrawzty Aug 17, 2018

phrawzty Aug 17, 2018

phrawzty Aug 17, 2018

zippolyte Aug 17, 2018

phrawzty Aug 17, 2018

phrawzty Aug 17, 2018

gmmeyer Aug 17, 2018

gmmeyer Aug 17, 2018

jeremy-lq Aug 17, 2018

jeremy-lq Aug 17, 2018

zippolyte commented Aug 20, 2018

ofek commented Aug 20, 2018

zippolyte left a comment


		[Run the Agent's `status` subcommand][5] and look for `{check_name}` under the Checks section.

		## Data Collected


		### Service Checks

		The {check_name_cap} check does not include any service checks at this time.


		### Events

		The {check_name_cap} check does not include any events at this time.


		Need help? Contact [Datadog Support][6].

		[1]: link to integration's site


		### Installation

		The {check_name_cap} check is included in the [Datadog Agent][2] package, so you don't

Add command to create new integrations #2037

Add command to create new integrations #2037

Conversation

ofek commented Aug 13, 2018 • edited Loading

Additional Notes

gmmeyer Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

jeremy-lq commented Aug 13, 2018

Choose a reason for hiding this comment

gzussa Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

gzussa left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zippolyte commented Aug 14, 2018

phrawzty commented Aug 14, 2018

ofek commented Aug 14, 2018

phrawzty commented Aug 14, 2018

ofek commented Aug 14, 2018

zippolyte commented Aug 14, 2018

ofek commented Aug 14, 2018

zippolyte left a comment

Choose a reason for hiding this comment

zippolyte Aug 17, 2018 • 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

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

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 Aug 20, 2018

ofek commented Aug 20, 2018

zippolyte left a comment

Choose a reason for hiding this comment

ofek commented Aug 13, 2018 •

edited

Loading

gmmeyer Aug 13, 2018 •

edited

Loading

gzussa Aug 13, 2018 •

edited

Loading

gzussa left a comment •

edited

Loading

zippolyte Aug 17, 2018 •

edited

Loading