Skip to content
This repository has been archived by the owner on May 25, 2022. It is now read-only.

file_input add mapstructure support #25

Merged

Conversation

wph95
Copy link
Member

@wph95 wph95 commented Feb 25, 2021

  • test
  • mapstructure
    • FileInputConfig
    • InputConfig
    • WriterConfig
    • BaseConfig
    • WriteTo

ref open-telemetry/opentelemetry-collector-contrib#2334
resolved #14

Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

This looks good to me. However, I would like to be very cautious with this change.

Before we can be confident that all unmarshaling is the same between previous yaml and now mapstructure, I believe we need to include some additional tests.

  1. We should have some "golden" config files, which are unmarshaled using both yaml and mapstructure, to ensure that the resulting config is exactly the same.
  2. Especially important, we need to ensure that custom types (those defined in helper package) are unmarshaled the same in both cases.

I have added another ticket here, which I will take myself: #33

operator/builtin/input/file/config_test.go Outdated Show resolved Hide resolved
@djaglowski djaglowski mentioned this pull request Mar 1, 2021
@djaglowski
Copy link
Member

We should have some "golden" config files, which are unmarshaled using both yaml and mapstructure, to ensure that the resulting config is exactly the same.

@wph95 I have merged #40, which I believe establishes a reasonable pattern for ensuring consistency between yaml and mapstructure unmarshaling. Would you mind pulling this in and including mapstructure validation of the same cases?

@wph95
Copy link
Member Author

wph95 commented Mar 4, 2021

We should have some "golden" config files, which are unmarshaled using both yaml and mapstructure, to ensure that the resulting config is exactly the same.

@wph95 I have merged #40, which I believe establishes a reasonable pattern for ensuring consistency between yaml and mapstructure unmarshaling. Would you mind pulling this in and including mapstructure validation of the same cases?

np, will try it tomorrow, #40 look great

@wph95 wph95 closed this Mar 5, 2021
@wph95 wph95 deleted the file-input-add-mapstructure-support branch March 5, 2021 05:21
@wph95 wph95 restored the file-input-add-mapstructure-support branch March 5, 2021 05:21
@wph95 wph95 reopened this Mar 5, 2021
Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

This looks excellent.

operator/builtin/input/file/config_test.go Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add mapstructure support to file_input operator configuration
2 participants