Update new integration templates to use v2 manifests #12592

ofek · 2022-07-26T05:06:53Z

Motivation

Move everything to new pipelines

codecov · 2022-07-26T05:11:50Z

Codecov Report

Merging #12592 (fdf25a4) into master (a48b506) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Flag	Coverage Δ
avi_vantage	`91.92% <ø> (ø)`
calico	`83.33% <ø> (ø)`
crio	`89.79% <ø> (ø)`
external_dns	`89.09% <ø> (ø)`
linux_proc_extras	`96.22% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

FlorentClarret

Looks good to me

mgarabed

👋 Bye, v1!

yzhan289

Just 1 small comment!

..._checks_dev/datadog_checks/dev/tooling/templates/integration/logs/{check_name}/manifest.json

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

nmuesch

Left an initial pass. Once those are addressed I can help run ddev create under various scenarios to make sure things look good before you merge 🙂

Excited to see V2 become the default!

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

nmuesch · 2022-07-26T20:51:07Z

..._checks_dev/datadog_checks/dev/tooling/templates/integration/tile/{check_name}/manifest.json

-  "type": "check",
-  "is_public": false,
-  "integration_id": "{integration_id}",{pricing_plan}{terms}{author_info}
+  "manifest_version": "2.0.0",


pricing_plan, terms, and author_info will need to continue living here. They should stay at the root of the manifest, probably below assets.

Right now it seems they are just - https://i.imgflip.com/3loyed.jpg

Wow a spoiler ... that's more heinous than force pushing to master

Btw, spoiler alert for ^^ Don't open if you haven't seen Avengers Endgame 🙊 😇

The damage is done my friend, I don't feel so good Mr. Muesch

🤨 🤨 that quote appears to be a spoiler ❗ ❗ ❗

nmuesch

I noticed two issues when testing, I left one inline about the author section.

The other issue is that creating a marketplace integration spits out an error:

<integration> is already on version `2.0.0`

I believe you'll need to remove the automatic triggering of the migrate command here - https://github.com/DataDog/integrations-core/blob/master/datadog_checks_dev/datadog_checks/dev/tooling/commands/create.py#L185-L186 (And likely can just remove that entire flag. I'm not sure how thats done in ddev releases, so maybe just removing the code check and removing the flag later? I'll leave up to you 🙂 )

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

nmuesch

Couple things I noticed while testing creating a new integration in the marketplace folder:

passing -t tile includes the full integration key inside assets. While that was needed with manifest V1, one of the big benefits of V2 is not requiring that for tile only listings (and therefore not requiring fields like source_type_name. Can we update it so that only the check type has this integration section please?

The migrate v2 command looks like it was adding some more fields than are present here. A few I noticed were in the author section, the vendor_id and sales_email -

integrations-core/datadog_checks_dev/datadog_checks/dev/tooling/manifest_validator/v2/migration.py

Lines 180 to 191 in 10c94eb

    
           elif repo_name == "marketplace": 
        
               migrated_manifest.set_path("/author/vendor_id", TODO_FILL_IN) 
        
               migrated_manifest.set_path("/author/sales_email", loaded_manifest.get_path("/terms/legal_email")) 
        
               migrated_manifest.set_path("/pricing", loaded_manifest.get_path("/pricing")) 
        
               migrated_manifest.set_path("/legal_terms", {}) 
        
               migrated_manifest.set_path("/legal_terms/eula", loaded_manifest.get_path("/terms/eula")) 
        
               for idx, _ in enumerate(migrated_manifest.get_path("/pricing") or []): 
        
                   migrated_manifest.set_path(f"/pricing/{idx}/product_id", TODO_FILL_IN) 
        
                   migrated_manifest.set_path(f"/pricing/{idx}/short_description", TODO_FILL_IN) 
        
                   migrated_manifest.set_path(f"/pricing/{idx}/includes_assets", True)

Can you please include those as well

nmuesch

As discussed offline @ofek I took a pass at making sure this was in line with other marketplace manifests. I left a minor commit but I think that this brings most of the major changes.

I think there's some quality-of-life things to figure out, because there still has to be manual tweaks to the generated manifest to get it to pass validations, but these modifications have to be made with more context about what the integration will do. As an example there's a generated empty dashboards key. If that truly stays empty, it needs to just be removed, but IMO its better to include during generation to know it exists than to remove it right now.

All that to say, I think this is a great step towards getting every new thing on Manifest V2, and we can iteratively update the command with smaller improvements over time 🙂

* Update new integration templates to use v2 manifests * address * address * address * address * Minor tweaks to emails Co-authored-by: Nick <nicholas.muesch@datadoghq.com> fd736e2

* Update new integration templates to use v2 manifests * address * address * address * address * Minor tweaks to emails Co-authored-by: Nick <nicholas.muesch@datadoghq.com>

ofek added the changelog/Added label Jul 26, 2022

ofek requested review from a team as code owners July 26, 2022 05:06

ghost added dev/tooling integration/datadog_checks_dev labels Jul 26, 2022

Update new integration templates to use v2 manifests

5bad2b5

ofek force-pushed the ofek/v2 branch from 75ea6e4 to 5bad2b5 Compare July 26, 2022 12:26

FlorentClarret previously approved these changes Jul 26, 2022

View reviewed changes

mgarabed previously approved these changes Jul 26, 2022

View reviewed changes

yzhan289 reviewed Jul 26, 2022

View reviewed changes

..._checks_dev/datadog_checks/dev/tooling/templates/integration/logs/{check_name}/manifest.json Show resolved Hide resolved

yzhan289 reviewed Jul 26, 2022

View reviewed changes

datadog_checks_dev/datadog_checks/dev/tooling/commands/create.py Show resolved Hide resolved

nmuesch requested changes Jul 26, 2022

View reviewed changes

address

3143821

ofek dismissed stale reviews from mgarabed and FlorentClarret via 3143821 July 26, 2022 22:28

steveny91 previously approved these changes Jul 28, 2022

View reviewed changes

nmuesch requested changes Jul 28, 2022

View reviewed changes

datadog_checks_dev/datadog_checks/dev/tooling/commands/create.py Outdated Show resolved Hide resolved

ofek dismissed steveny91’s stale review via 2de2b34 July 28, 2022 18:32

address

55adcd5

ofek force-pushed the ofek/v2 branch from 2de2b34 to 55adcd5 Compare July 28, 2022 19:44

yzhan289 previously approved these changes Aug 1, 2022

View reviewed changes

nmuesch reviewed Aug 3, 2022

View reviewed changes

buraizu previously approved these changes Aug 3, 2022

View reviewed changes

address

715a8ea

ofek dismissed stale reviews from buraizu and yzhan289 via 715a8ea August 3, 2022 22:11

ghost added the documentation label Aug 3, 2022

ofek and others added 2 commits August 10, 2022 18:35

address

31a1da8

Minor tweaks to emails

fdf25a4

nmuesch approved these changes Aug 11, 2022

View reviewed changes

ofek merged commit fd736e2 into master Aug 18, 2022

ofek deleted the ofek/v2 branch August 18, 2022 02:20

FlorentClarret mentioned this pull request Oct 10, 2022

Add the Do Not Upload classifier to all integrations #13066

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update new integration templates to use v2 manifests #12592

Update new integration templates to use v2 manifests #12592

ofek commented Jul 26, 2022

codecov bot commented Jul 26, 2022 •

edited

Loading

FlorentClarret left a comment

mgarabed left a comment

yzhan289 left a comment

nmuesch left a comment

nmuesch Jul 26, 2022

ofek Jul 26, 2022

nmuesch Jul 26, 2022

ofek Jul 26, 2022

nmuesch Jul 26, 2022

nmuesch left a comment

nmuesch left a comment

nmuesch left a comment

	elif repo_name == "marketplace":
	migrated_manifest.set_path("/author/vendor_id", TODO_FILL_IN)
	migrated_manifest.set_path("/author/sales_email", loaded_manifest.get_path("/terms/legal_email"))

	migrated_manifest.set_path("/pricing", loaded_manifest.get_path("/pricing"))
	migrated_manifest.set_path("/legal_terms", {})
	migrated_manifest.set_path("/legal_terms/eula", loaded_manifest.get_path("/terms/eula"))

	for idx, _ in enumerate(migrated_manifest.get_path("/pricing") or []):
	migrated_manifest.set_path(f"/pricing/{idx}/product_id", TODO_FILL_IN)
	migrated_manifest.set_path(f"/pricing/{idx}/short_description", TODO_FILL_IN)
	migrated_manifest.set_path(f"/pricing/{idx}/includes_assets", True)

Update new integration templates to use v2 manifests #12592

Update new integration templates to use v2 manifests #12592

Conversation

ofek commented Jul 26, 2022

Motivation

codecov bot commented Jul 26, 2022 • edited Loading

Codecov Report

FlorentClarret left a comment

Choose a reason for hiding this comment

mgarabed left a comment

Choose a reason for hiding this comment

yzhan289 left a comment

Choose a reason for hiding this comment

nmuesch left a comment

Choose a reason for hiding this comment

nmuesch Jul 26, 2022

Choose a reason for hiding this comment

ofek Jul 26, 2022

Choose a reason for hiding this comment

nmuesch Jul 26, 2022

Choose a reason for hiding this comment

ofek Jul 26, 2022

Choose a reason for hiding this comment

nmuesch Jul 26, 2022

Choose a reason for hiding this comment

nmuesch left a comment

Choose a reason for hiding this comment

nmuesch left a comment

Choose a reason for hiding this comment

nmuesch left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 26, 2022 •

edited

Loading