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.

Sign up for GitHub

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

Jump to bottom

[BUG-255] - Fixing Issues with pipeline_metadata #256

Merged
merged 6 commits into from
Feb 6, 2023
Merged

[BUG-255] - Fixing Issues with pipeline_metadata #256

merged 6 commits into from
Feb 6, 2023

Conversation

preyas-prakasan
Copy link
Contributor

@preyas-prakasan preyas-prakasan commented Jan 20, 2023 •
edited by tobio
Loading

Fixes #255

@cla-checker-service
Copy link

cla-checker-service bot commented Jan 20, 2023 •
edited
Loading

💚 CLA has been signed

@preyas-prakasan
Copy link
Contributor Author

@tobio , could you unblock this for me please. This is my first PR and hope to fix the bug raised in the Issues area.

tobio
tobio reviewed Jan 23, 2023
View reviewed changes
internal/elasticsearch/logstash/pipeline.go Outdated
@@ -71,7 +71,7 @@ func ResourceLogstashPipeline() *schema.Resource {
Optional: true,
Elem: &schema.Schema{
Type: schema.TypeString,
Default: nil,
Default: "{}",
Copy link
Member

Choose a reason for hiding this comment

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

This sets a default value for individual metadata keys, not the entire metadata map. You likely want to define DiffSuppressFunc on the actual pipeline_metadata schema object.

tobio
tobio reviewed Jan 23, 2023
View reviewed changes
Copy link
Member

@tobio tobio left a comment

Choose a reason for hiding this comment

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

It feels like we should be able to include an acceptance test demonstrating the issue (and the fix) with this PR.

@preyas-prakasan preyas-prakasan changed the title replacing nil with null to check [BUG-255] - Fixing Issues with pipeline_metadata Jan 24, 2023
@preyas-prakasan
Copy link
Contributor Author

Thanks for the comments and guidance @tobio . Could you see if this is what you intended ? Appreciate the guidance as I'm starting my journey with Go here.

tobio
tobio reviewed Feb 6, 2023
View reviewed changes
Copy link
Member

@tobio tobio left a comment

Choose a reason for hiding this comment

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

Thanks for this @preyas-prakasan! Sorry about the delay in getting a review here.

@tobio tobio enabled auto-merge (squash) February 6, 2023 08:51
@tobio tobio merged commit 8462101 into elastic:main Feb 6, 2023
@preyas-prakasan
Copy link
Contributor Author

Thanks for this @preyas-prakasan! Sorry about the delay in getting a review here.

No problem at all. Thanks for the guidance @tobio.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tobio tobio tobio approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug] - Logstash pipeline metadata detected as a change during second run
2 participants
@preyas-prakasan @tobio