Skip to content

Migrating traefik module#763

Merged
ycombinator merged 25 commits intoelastic:masterfrom
ycombinator:migrate-traefik
Mar 11, 2021
Merged

Migrating traefik module#763
ycombinator merged 25 commits intoelastic:masterfrom
ycombinator:migrate-traefik

Conversation

@ycombinator
Copy link
Copy Markdown
Contributor

@ycombinator ycombinator commented Mar 5, 2021

What does this PR do?

Migrates the traefik module from Beats to Integrations.

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.

Screenshots

kibana-traefik

Related issues

@elasticmachine
Copy link
Copy Markdown

elasticmachine commented Mar 5, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Pull request #763 updated

  • Start Time: 2021-03-10T22:43:49.442+0000

  • Duration: 51 min 14 sec

  • Commit: c2a87a3

Test stats 🧪

Test Results
Failed 0
Passed 16
Skipped 0
Total 16

Trends 🧪

Image of Build Times

Image of Tests

@andresrc andresrc added the Team:Integrations Label for the Integrations team label Mar 6, 2021
@ycombinator ycombinator mentioned this pull request Mar 9, 2021
17 tasks
@ycombinator ycombinator marked this pull request as ready for review March 9, 2021 01:57
@elasticmachine
Copy link
Copy Markdown

Pinging @elastic/integrations (Team:Integrations)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... GA?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing you want me to change it to experimental so it matches up with the release in the package-level manifest.yml?

BTW, this is what was generated by the migration script. Should we update it to generate release: experimental?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's because originally integrations were supposed to be 1:1 modules. If a module was released GA, so the integration too. You can open an issue, but I'm not sure about it's priority (rather low, it's easy to adjust after migration).

Copy link
Copy Markdown
Contributor Author

@ycombinator ycombinator Mar 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized that this release: ga is in the fields.yml, not the manifest.yml. I'm fixing it to be release: experimental but I'm seeing some other packages (e.g. nats, docker) where the release is set to ga in their fields.yml but the package manifest.yml has release: experimental or release: beta.

Copy link
Copy Markdown
Contributor Author

@ycombinator ycombinator Mar 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I ended up just removing the release field from this fields.yml file (c857182d). I checked the spec for field definition files and that field is not even mentioned in there. So I'm not even sure of it's purpose or if we need it. 🤷

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose that you didn't check compatibility with a more recent version?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, not yet. In this PR I'm just focussing on the basic migration. I will make a follow up PR to test with a more recent version and then make the necessary updates to the package.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore my previous comment. I updated the version of Traefik in this PR to 1.7.

There is a 2.x version series as well but I think we should leave that for a follow up PR as it might contain breaking changes. WDYT?

@mtojek
Copy link
Copy Markdown
Contributor

mtojek commented Mar 10, 2021

jenkins run the tests please

@mtojek mtojek self-requested a review March 10, 2021 08:51
Copy link
Copy Markdown
Contributor

@mtojek mtojek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the only concern I have is to take a fresh screenshot. You can also put one in the issue description. @sorantis used to give suggestions what should be improved there.

@ycombinator ycombinator merged commit 2d0e5f7 into elastic:master Mar 11, 2021
@ycombinator ycombinator deleted the migrate-traefik branch March 11, 2021 15:28
@andrewkroh andrewkroh added Integration:traefik Traefik New Integration Issue or pull request for creating a new integration package. labels Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Integration:traefik Traefik New Integration Issue or pull request for creating a new integration package. Team:Integrations Label for the Integrations team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants