Keep track of the checks changed at every Datadog Agent release #2277

masci · 2018-09-20T12:54:56Z

What does this PR do?

Adds the release agent_changelog command to ddev to provide a list of the changes for every Agent release in the form of a changelog file.

Motivation

The Agent changelog points to an invalid list of changed checks, from now on it can point directly the AGENT_CHANGELOG.md file from the root of this repo.

Review checklist

PR has a meaningful title or PR has the no-changelog label attached
Feature or bugfix has tests
Git history is clean
If PR impacts documentation, docs team has been notified or an issue has been opened on the documentation repo

Additional Notes

Generating the changelog is still a manual procedure!

phrawzty · 2018-09-20T13:04:08Z

Failed on flake8?

Also - and this is a minor thing - but I think we can drop the trailing period following the version numbers.

l0k0ms

Very nice! this new format is simply, effective, and easily searchable!

I just have a couple of feedback about the wording to make it more obvious for the user when scrolling fast in the changelog.

l0k0ms · 2018-09-20T13:08:11Z

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

+    # go through all the agent releases
+    for agent, changes in changes_per_agent.iteritems():
+        url = agent_changelog_url.format(agent)
+        changelog.write('## Datadog Agent [{}]({})\n\n'.format(agent, url))


Should we say ## Datadog Agent version %% ?

Also, do we want to also specify the Agent version release date?

Showing the date is tricky because it would be the date the tag was added on integrations-core, that might be slightly different from the real one

l0k0ms · 2018-09-20T13:10:13Z

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

+            changelog.write('* There were no check updates for this version of the Agent.\n\n')
+        else:
+            for name, ver in changes.iteritems():
+                breaking_notice = "This is a breaking change." if ver[1] else ""


We should make the breaking notice more obvious I think. What do you think of BREAKING CHANGE ?

…ious

codecov-io · 2018-09-20T13:32:40Z

Codecov Report

Merging #2277 into master will decrease coverage by 10.81%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #2277       +/-   ##
===========================================
- Coverage   85.66%   74.84%   -10.82%     
===========================================
  Files         188       17      -171     
  Lines       13254      493    -12761     
  Branches     1435       57     -1378     
===========================================
- Hits        11354      369    -10985     
+ Misses       1493      105     -1388     
+ Partials      407       19      -388

zippolyte · 2018-09-20T13:41:30Z

datadog_checks_dev/datadog_checks/dev/tooling/utils.py

+        if len(toks) != 2 or not toks[0] or not toks[1]:
+            # if we get here, the requirements file is garbled but let's stay
+            # resilient
+            continue


Do we want to print a message as well just in case ?

This is only necessary because we checked in and tagged a file with a git conflict at some point 🙈

Now that the changes to the reqfile are automated, this shouldn't happen again and if I log something we'll get the warning from that one tag forever

gzussa

Awesome tool!

gzussa · 2018-09-20T13:48:49Z

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

+@click.pass_context
+def agent_changelog(ctx, since, to, output, force):
+    """
+    FIXME


you know what i think about FIXME and TODO in code 😝

gzussa · 2018-09-20T13:52:13Z

datadog_checks_dev/datadog_checks/dev/tooling/git.py

+
+
+def git_tag_list(pattern=None):
+    with chdir(get_root()):


Could you add some descriptions here as well

gzussa · 2018-09-20T13:53:38Z

datadog_checks_dev/datadog_checks/dev/tooling/utils.py

+def parse_agent_req_file(contents):
+    """
+    Returns a dictionary mapping {check_name --> pinned_version} from the
+    given file contents. We can assume lines are in the form:


Shouldn't we have a small validation just in case the assumption fails for whatever reasons

That's pip syntax, it's a safe assumption to make for now since the requirements file is auto generated.

gzussa · 2018-09-20T13:55:02Z

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

+    # reverse so we have descendent order
+    agent_tags = agent_tags[::-1]
+
+    # store the changes in a mapping {agent_version --> {check_name --> current_version}}


Thanks for the comment 👍

gzussa · 2018-09-20T13:58:16Z

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

+    changelog = StringIO()
+
+    # prepare the links
+    agent_changelog_url = 'https://github.com/DataDog/datadog-agent/blob/master/CHANGELOG.rst#{}'


I am guessing we don't support agent 5?

Correct, we only support Agent 6.3.0+

gzussa · 2018-09-20T13:59:39Z

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

+        changelog.write('## Datadog Agent version [{}]({})\n\n'.format(agent, url))
+
+        if not changes:
+            changelog.write('* There were no check updates for this version of the Agent.\n\n')


should we call our stuff check, or integration or maybe even integration check?

gzussa · 2018-09-20T14:02:26Z

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

+            for name, ver in changes.iteritems():
+                breaking_notice = " **BREAKING CHANGE** " if ver[1] else ""
+                changelog_url = check_changelog_url.format(name)
+                changelog.write('* {} [{}]({}){}\n'.format(name, ver[0], changelog_url, breaking_notice))


I understand it may be a little bit more complex to implement. However do we want to write the integration name or the integration folder name. For example MySQL instead mysql?

gzussa · 2018-09-20T14:03:04Z

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

+            changelog.write('\n')
+
+    # save the changelog on disk if --output was passed
+    if output:


Could you add a dry run param that just output to stdout?

It's what it does if you don't pass -o (see the else branch)

gzussa

Looks pretty good to me now. Thanks 🥇

masci added 3 commits September 19, 2018 20:44

first draft

7b5dda6

changed the CLI, automatically recreate the whole changelog by default

1a890ea

checking in the agent changelog

ff3fb50

masci requested review from a team as code owners September 20, 2018 12:54

masci added documentation dev/tooling labels Sep 20, 2018

fix links to agent changelog

13b8f6e

l0k0ms reviewed Sep 20, 2018

View reviewed changes

masci added 2 commits September 20, 2018 15:23

fix flake8

05b1787

remove dot after version number, make breaking change notice more obv…

183a934

…ious

add 'version' to the header

b62c327

zippolyte reviewed Sep 20, 2018

View reviewed changes

masci added changelog/Added integration/datadog_checks_dev labels Sep 20, 2018

gzussa suggested changes Sep 20, 2018

View reviewed changes

addressed PR comments and updated the changelog

7b00e52

gzussa approved these changes Sep 20, 2018

View reviewed changes

masci merged commit 27629cc into master Sep 20, 2018

masci deleted the massi/changelog branch September 20, 2018 15:54

ofek mentioned this pull request Sep 20, 2018

Fix for Python 3 #2281

Merged

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

Keep track of the checks changed at every Datadog Agent release (#2277)

a60b1da

Keep track of the checks changed at every Datadog Agent release #2277

Keep track of the checks changed at every Datadog Agent release #2277

Uh oh!

Conversation

masci commented Sep 20, 2018

What does this PR do?

Motivation

Review checklist

Additional Notes

Uh oh!

phrawzty commented Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

l0k0ms left a comment

Choose a reason for hiding this comment

Uh oh!

l0k0ms Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Sep 20, 2018

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gzussa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gzussa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

phrawzty commented Sep 20, 2018 •

edited

Loading

l0k0ms Sep 20, 2018 •

edited

Loading