integrations-core icon indicating copy to clipboard operation
integrations-core copied to clipboard

[cert_manager] Migrate from integrations-extras

Open davidor opened this issue 2 years ago • 12 comments

What does this PR do?

Migrates the cert-manager integration from integrations-extras.

I have not changed the code. All credit goes to @arapulido who created the integration and all the other contributors.

I just adapted a few things like the install instructions in the README, the maintainer email, etc.

This is my first time migrating an integration from the integrations-extras repo, so I have some questions:

  • I have set 1.0.0 as the version number and reset the CHANGELOG. Is that OK or should we continue from the version in integrations-extras?
  • I've changed the author to Datadog because I've seen that's what we use in the integrations of this repo. Is that OK or should we keep the original in this case?

Review checklist (to be filled by reviewers)

  • [ ] Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • [ ] PR title must be written as a CHANGELOG entry (see why)
  • [ ] Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • [ ] PR must have changelog/ and integration/ labels attached

davidor avatar Jun 08 '22 08:06 davidor

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

github-actions[bot] avatar Jun 08 '22 08:06 github-actions[bot]

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

github-actions[bot] avatar Jun 08 '22 08:06 github-actions[bot]

Codecov Report

Merging #12344 (39a8098) into master (a05ca18) will decrease coverage by 0.01%. The diff coverage is 77.41%.

Flag Coverage Δ
cert_manager 77.41% <77.41%> (?)

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

codecov[bot] avatar Jun 08 '22 08:06 codecov[bot]

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

github-actions[bot] avatar Jun 08 '22 08:06 github-actions[bot]

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

github-actions[bot] avatar Jun 08 '22 08:06 github-actions[bot]

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

github-actions[bot] avatar Jun 08 '22 08:06 github-actions[bot]

Can you add container integrations as codeowners for this?

hithwen avatar Jun 09 '22 08:06 hithwen

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

github-actions[bot] avatar Jun 10 '22 10:06 github-actions[bot]

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

github-actions[bot] avatar Jun 10 '22 10:06 github-actions[bot]

@sarah-witt Thanks for the review. I've addressed your comments:

  • Deleted requirements.in.
  • Deleted MANIFEST.in.
  • Updated setup.py with some things that were missing from the original one. I wasn't sure about what's no longer needed.
  • Removed the pip install -r requirements.in line from tox.ini.
  • Updated the datadog-checks-base dependency requirement to >=25.1.0. I don't know which version we should use exactly, so I chose this one which is the one that most integrations are using.
  • Added the spec in manifest.json.
  • Regenerated the example spec. My understading is that this integration should use openmetrics_legacy because I've seen that it accepts prometheus_url as a config param.

I've added each thing in a different commit.

davidor avatar Jun 10 '22 11:06 davidor

Reference

Has this been addressed? I think we should still bump the version to generate a new release from core, but we should definetly preserve prior history

hithwen avatar Jul 04 '22 10:07 hithwen

@hithwen I added a new commit that brings the changelog from integrations-extras and continues with the same version number.

davidor avatar Aug 01 '22 15:08 davidor

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

github-actions[bot] avatar Sep 06 '22 15:09 github-actions[bot]

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

github-actions[bot] avatar Sep 06 '22 15:09 github-actions[bot]

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

github-actions[bot] avatar Sep 06 '22 15:09 github-actions[bot]

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

github-actions[bot] avatar Sep 07 '22 09:09 github-actions[bot]

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

github-actions[bot] avatar Sep 07 '22 09:09 github-actions[bot]

The CI is failing because there's a check that detects that there's already an integration with the same name, which is expected because we know that it exists in the integrations-extras repo.

Apart from that, I pushed 3 new commits that were added after the last approval by @FlorentClarret

  • Migrate to OpenMetrics V2 (as suggested by @ofek ).
  • Adapt to changes in tooling.
  • Migrate manifest to v2.0.0 (it's now required to make the tests pass).

davidor avatar Sep 09 '22 13:09 davidor

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

github-actions[bot] avatar Sep 13 '22 17:09 github-actions[bot]

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

github-actions[bot] avatar Sep 13 '22 17:09 github-actions[bot]

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

github-actions[bot] avatar Sep 14 '22 15:09 github-actions[bot]