Skip to content

[awsfirehose] remove redudant .logs suffix from dataset name - #7670

Merged
tommyers-elastic merged 3 commits into
mainfrom
change-firehose-logs-dataset-name
Sep 21, 2023
Merged

[awsfirehose] remove redudant .logs suffix from dataset name#7670
tommyers-elastic merged 3 commits into
mainfrom
change-firehose-logs-dataset-name

Conversation

@tommyers-elastic

Copy link
Copy Markdown
Contributor

What does this PR do?

The dataset name is awsfirehose.logs, but the datastream type is logs. so the datastream name is logs-awsfirehose.logs-*. This PR removes this redundancy.

The package is in very early tech preview - we are not worries about this being a major breaking change.

@tommyers-elastic tommyers-elastic added the enhancement New feature or request label Sep 6, 2023
@tommyers-elastic
tommyers-elastic requested a review from a team as a code owner September 6, 2023 08:23
@elasticmachine

elasticmachine commented Sep 6, 2023

Copy link
Copy Markdown

💚 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-21T15:04:03.572+0000

  • Duration: 16 min 50 sec

Test stats 🧪

Test Results
Failed 0
Passed 9
Skipped 0
Total 9

🤖 GitHub comments

Expand to view the GitHub comments

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

  • /test : Re-trigger the build.

@elasticmachine

elasticmachine commented Sep 6, 2023

Copy link
Copy Markdown

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (1/1) 💚
Files 100.0% (1/1) 💚
Classes 100.0% (1/1) 💚
Methods 60.0% (3/5)
Lines 100.0% (79/79) 💚
Conditionals 100.0% (0/0) 💚
@tommyers-elastic

Copy link
Copy Markdown
Contributor Author

closing this due to technical limitation and uncertainty around whether or not this is the right thing to do. the logs suffix is likely here to stay.

ruflin added a commit to ruflin/elastic-package that referenced this pull request Sep 8, 2023
The dataset value in the manifest.yml file can be overwritten to allow dataset names which are not `{package}.{dirName}`. One example of this can be found here: elastic/integrations#7670 This is already in use today and is used by some packages. Fleet installs all the assets as expected.

When working on elastic/integrations#7670, CI is failing. The reason is that the pipeline check did not take the custom dataset config into account. The reason why this didn't show up earlier is because the check for the template already has a similar condition in place. Likely all previous packages that used this feature did only have templates and no pipelines.
@ruflin

ruflin commented Sep 8, 2023

Copy link
Copy Markdown
Contributor

I have a fix for this opened here: elastic/elastic-package#1437

@ruflin ruflin added the blocked label Sep 11, 2023
ruflin added a commit to elastic/elastic-package that referenced this pull request Sep 11, 2023
The dataset value in the manifest.yml file can be overwritten to allow dataset names which are not `{package}.{dirName}`. One example of this can be found here: elastic/integrations#7670 This is already in use today and is used by some packages. Fleet installs all the assets as expected.

When working on elastic/integrations#7670, CI is failing. The reason is that the pipeline check did not take the custom dataset config into account. The reason why this didn't show up earlier is because the check for the template already has a similar condition in place. Likely all previous packages that used this feature did only have templates and no pipelines.
@ruflin

ruflin commented Sep 11, 2023

Copy link
Copy Markdown
Contributor

@tommyers-elastic elastic/elastic-package#1437 has landed. To get this PR green, a new elastic-package release is needed. Can you and team help to get this over the line?

@ruflin

ruflin commented Sep 13, 2023

Copy link
Copy Markdown
Contributor

The newest version of elastic-package was just realised with the fix in places. Lets rerun CI on this PR.

@tommyers-elastic
tommyers-elastic requested a review from a team as a code owner September 13, 2023 12:20
@tommyers-elastic
tommyers-elastic force-pushed the change-firehose-logs-dataset-name branch from 280c9e4 to 2e13cf3 Compare September 13, 2023 15:16

@kaiyan-sheng kaiyan-sheng left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for making the change!

@kaiyan-sheng

Copy link
Copy Markdown

/test

@kaiyan-sheng kaiyan-sheng left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@tommyers-elastic
tommyers-elastic force-pushed the change-firehose-logs-dataset-name branch from 82c6a67 to a85ebcd Compare September 18, 2023 14:19
@tommyers-elastic
tommyers-elastic force-pushed the change-firehose-logs-dataset-name branch from a85ebcd to fadf832 Compare September 18, 2023 14:20
@kaiyan-sheng

Copy link
Copy Markdown
[2023-09-20T15:43:05.650Z] Error response from daemon: manifest for docker.elastic.co/kibana/kibana:8.10.0 not found: manifest unknown: manifest unknown

This is caused by kibana 8.10.0 image was removed.

bhapas pushed a commit to bhapas/elastic-package that referenced this pull request Sep 21, 2023
The dataset value in the manifest.yml file can be overwritten to allow dataset names which are not `{package}.{dirName}`. One example of this can be found here: elastic/integrations#7670 This is already in use today and is used by some packages. Fleet installs all the assets as expected.

When working on elastic/integrations#7670, CI is failing. The reason is that the pipeline check did not take the custom dataset config into account. The reason why this didn't show up earlier is because the check for the template already has a similar condition in place. Likely all previous packages that used this feature did only have templates and no pipelines.
@tommyers-elastic
tommyers-elastic merged commit 1cf26c3 into main Sep 21, 2023
@tommyers-elastic
tommyers-elastic deleted the change-firehose-logs-dataset-name branch September 21, 2023 15:37
@elasticmachine

Copy link
Copy Markdown

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

@andrewkroh andrewkroh added the Integration:awsfirehose Amazon Data Firehose label Jul 22, 2024
orestisfl pushed a commit to orestisfl/integrations that referenced this pull request May 15, 2026
Co-authored-by: kaiyan-sheng <kaiyan.sheng@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request Integration:awsfirehose Amazon Data Firehose

7 participants