Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Barracuda_WAF] Change the package name #7735

Merged
merged 7 commits into from Oct 3, 2023

Conversation

vinit-chauhan
Copy link
Contributor

@vinit-chauhan vinit-chauhan commented Sep 8, 2023

What does this PR do?

As mentioned in issue, #7724 the package name of Barracuda needed to be changed. This PR, therefore changes the name of the package and all of its associated assets (package name, data_stream name, dashboard IDs, field names, etc. )

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.
  • I have verified that Kibana version constraints are current according to guidelines.

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Screenshots

image
image

@elasticmachine
Copy link

elasticmachine commented Sep 8, 2023

💚 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 preview

Expand to view the summary

Build stats

  • Start Time: 2023-09-29T12:41:10.576+0000

  • Duration: 18 min 54 sec

Test stats 🧪

Test Results
Failed 0
Passed 18
Skipped 0
Total 18

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@efd6
Copy link
Contributor

efd6 commented Sep 11, 2023

/test

@vinit-chauhan
Copy link
Contributor Author

Hey @efd6 - I've added CODEOWNER entry. Would you mind running the tests again? 😄

@vinit-chauhan
Copy link
Contributor Author

vinit-chauhan commented Sep 11, 2023

Most of the changes are done for this PR. However, I'll waiting for @jamiehynds' input on the issue. Once done, I'll mark it to "Ready to review".

@efd6
Copy link
Contributor

efd6 commented Sep 11, 2023

/test

@elasticmachine
Copy link

elasticmachine commented Sep 11, 2023

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (1/1) 💚
Files 100.0% (6/6) 💚
Classes 100.0% (6/6) 💚
Methods 100.0% (36/36) 💚
Lines 96.597% (369/382)
Conditionals 100.0% (0/0) 💚

@jamiehynds
Copy link

Most of the changes are done for this PR. However, I'll waiting for @jamiehynds' input on the issue. Once done, I'll mark it to "Ready to review".

Thanks @vinit-chauhan - looking at the package version, the manifest.yml mentions 0.1.0 but the UI is showing 1.5.0. Any ideas as to why there's a discrepancy?

Screenshot 2023-09-11 at 11 30 09

@vinit-chauhan
Copy link
Contributor Author

Hey @jamiehynds - looks like the one you are referring to is the old integration. I've attached the screenshot along with the PR, which has updated Title and Readme. I'll have to update the integration version and titles of Visualizations if required to be aligned with the new name.

@vinit-chauhan vinit-chauhan marked this pull request as ready for review September 12, 2023 01:19
@vinit-chauhan
Copy link
Contributor Author

Hey @jamiehynds - Updated the PR to match the version per your suggestion on the issue. And marking the PR ready for review. 😄

@jamiehynds
Copy link

jamiehynds commented Sep 12, 2023

Hey @jamiehynds - looks like the one you are referring to is the old integration. I've attached the screenshot along with the PR, which has updated Title and Readme. I'll have to update the integration version and titles of Visualizations if required to be aligned with the new name.

Thanks @vinit-chauhan - I don't think you attached the screenshot, would you mind attaching? I'm looking at the latest version of the integration, which is appearing as 1.5.0.

@bhapas worked on the WAF integration. Can you remember if you just replaced the existing datastream within the Barracuda integration?

@vinit-chauhan
Copy link
Contributor Author

vinit-chauhan commented Sep 12, 2023

Hey @jamiehynds - Here's the Screenshot.

image

In the older integration of barracuda v1.5.0, there was only one data stream named waf. In this PR, the package has been renamed to barracuda_waf and data-stream to logs. Moreover, the older barracuda integration is not removed in this PR. Therefore, there are 2 integrations.

If you want I can remove the older integrations and add this one or we can add an update to the old integration with a message that "we are moving to a new integration, and users would have to switch to the new integration." and later deprecate/remove it.

If we want users to allow seamless updates, We will have to keep the package name the same. which is "barracuda". If we update the package name, to barracuda_waf it will create a new entry in the package registry.

So there are two possible approaches to tackle this,

Approach 1: Rename the integration package to barracuda_waf: This would help us maintain the integration uniformity in the code base and in data ( field name for the custom field would follow package_name.data-stream_name: barracuda_waf.logs ) as the naming convention would be the same as other integrations. However, it would result in a somewhat bad UX, as existing users would have move to new integration and reindex old data, instead of simply clicking on the Update button for this particular update.

Approach 2: Use the existing package name, It would allow the users to update the package with ease. However, on the other hand, it would break the conversion in the code base and field mappings ( barracuda.waf )

Note: In this PR, I've used Approach 1.

Let me know your thoughts, and the changes necessary.

@kush-elastic
Copy link
Collaborator

/test

@bhapas
Copy link
Contributor

bhapas commented Sep 28, 2023

@jamiehynds @vinit-chauhan

Looks like there is a confusion here? I think issue #7724 meant to change the title and description in the manifest.yml - @jamiehynds correct me if I am wrong

title: "Barracuda Logs"
version: "1.8.0"
description: Ingest Events from Barracuda Web Application Firewall

@jamiehynds
Copy link

Thanks @bhapas - correct, just wanted to adjust the title to ensure it's clear the integration only covers WAF events. The title was originally 'Barracuda Logs' due to multiple datastreams for different products within that integration, it's now just limited to WAF so hoping to adjust the title accordingly.

@bhapas
Copy link
Contributor

bhapas commented Sep 28, 2023

Thanks @jamiehynds ..

@vinit-chauhan , So this is just down to

integrations/packages/barracuda/manifest.yml
- title: "Barracuda Logs"
- description: Ingest Events from Barracuda Web Application Firewall
+ title: "Barracuda Web Application Firewall"
+ description: Collect logs from Barracuda Web Application Firewall with Elastic Agent.

@vinit-chauhan
Copy link
Contributor Author

Ah, I see. I thought we were going to update the whole package. So it's just the Manifest file then.

Thanks for the update, I'll do the changes later today. 😄

@vinit-chauhan vinit-chauhan requested a review from a team as a code owner September 28, 2023 19:25
@bhapas
Copy link
Contributor

bhapas commented Sep 29, 2023

/test

@efd6 efd6 merged commit 1fe6dae into elastic:main Oct 3, 2023
4 checks passed
@elasticmachine
Copy link

Package barracuda - 1.9.0 containing this change is available at https://epr.elastic.co/search?package=barracuda

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Barracuda] Rename to Barracuda WAF
6 participants