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

[cmd/opampsupervisor] Enable Strict Unmarshal for Supervisor Configuration #36148

Merged
merged 11 commits into from
Nov 27, 2024

Conversation

akstron
Copy link
Contributor

@akstron akstron commented Nov 2, 2024

Description

The changes includes providing a custom DecoderConfig with ErrorUnused enabled instead of using the default DecoderConfig provided by koanf

Link to tracking issue

Fixes: #35838

…efault provided by koanf

Signed-off-by: Alok Kumar Singh <[email protected]>
Copy link

linux-foundation-easycla bot commented Nov 2, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@akstron akstron changed the title [cmd/opampsupervisor] Enable Strictly Unmarshal for Supervisor Configuration [cmd/opampsupervisor] Enable Strict Unmarshal for Supervisor Configuration Nov 2, 2024
@akstron
Copy link
Contributor Author

akstron commented Nov 6, 2024

Hi @evan-bradley / @atoulme / @tigrannajaryan can you please help review this? Thanks

@atoulme
Copy link
Contributor

atoulme commented Nov 9, 2024

Please add a changelog - any chance you can have a test checking the behavior?

Signed-off-by: Alok Kumar Singh <[email protected]>
Signed-off-by: Alok Kumar Singh <[email protected]>
@akstron
Copy link
Contributor Author

akstron commented Nov 9, 2024

Hi @atoulme , added both chlog and test in recent commits. Thanks

Signed-off-by: Alok Kumar Singh <[email protected]>
Signed-off-by: Alok Kumar Singh <[email protected]>
@akstron
Copy link
Contributor Author

akstron commented Nov 19, 2024

Hi @atoulme , just following up on this PR. Let me know if there are any concerns or updates? Thanks

Copy link
Contributor

@fatsheep9146 fatsheep9146 left a comment

Choose a reason for hiding this comment

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

Please fix the failed CI lint https://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/11762947657/job/33410954056?pr=36148

--- a/cmd/opampsupervisor/go.mod
+++ b/cmd/opampsupervisor/go.mod
@@ -4,6 +4,7 @@ go 1.22.0
 
 require (
 	github.com/cenkalti/backoff/v4 v4.3.0
+	github.com/go-viper/mapstructure/v2 v2.0.0-alpha.1
 	github.com/google/uuid v1.6.0
 	github.com/knadh/koanf/maps v0.1.1
 	github.com/knadh/koanf/parsers/yaml v0.1.0
@@ -25,7 +26,6 @@ require (
 require (
 	github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect
 	github.com/fsnotify/fsnotify v1.8.0 // indirect
-	github.com/go-viper/mapstructure/v2 v2.0.0-alpha.1 // indirect
 	github.com/google/go-cmp v0.6.0 // indirect
 	github.com/gorilla/websocket v1.5.1 // indirect
 	github.com/mitchellh/copystructure v1.2.0 // indirect
go.mod/go.sum deps changes detected, please run "make gotidy" and commit the changes in this PR.

@akstron
Copy link
Contributor Author

akstron commented Nov 23, 2024

Please fix the failed CI lint https://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/11762947657/job/33410954056?pr=36148

--- a/cmd/opampsupervisor/go.mod
+++ b/cmd/opampsupervisor/go.mod
@@ -4,6 +4,7 @@ go 1.22.0
 
 require (
 	github.com/cenkalti/backoff/v4 v4.3.0
+	github.com/go-viper/mapstructure/v2 v2.0.0-alpha.1
 	github.com/google/uuid v1.6.0
 	github.com/knadh/koanf/maps v0.1.1
 	github.com/knadh/koanf/parsers/yaml v0.1.0
@@ -25,7 +26,6 @@ require (
 require (
 	github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect
 	github.com/fsnotify/fsnotify v1.8.0 // indirect
-	github.com/go-viper/mapstructure/v2 v2.0.0-alpha.1 // indirect
 	github.com/google/go-cmp v0.6.0 // indirect
 	github.com/gorilla/websocket v1.5.1 // indirect
 	github.com/mitchellh/copystructure v1.2.0 // indirect
go.mod/go.sum deps changes detected, please run "make gotidy" and commit the changes in this PR.

Done, as instructed in the result.

Copy link
Contributor

@fatsheep9146 fatsheep9146 left a comment

Choose a reason for hiding this comment

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

please help resolve the conflict

@akstron
Copy link
Contributor Author

akstron commented Nov 24, 2024

please help resolve the conflict

Resolved the conflict. Thanks

Signed-off-by: Alok Kumar Singh <[email protected]>
@akstron
Copy link
Contributor Author

akstron commented Nov 25, 2024

Hi @fatsheep9146 , I fixed the pr check failure. Can you please re-trigger the checks?

@fatsheep9146
Copy link
Contributor

Ping @atoulme @evan-bradley for a final review.

.chloggen/strict-unmarshal.yaml Outdated Show resolved Hide resolved
.chloggen/strict-unmarshal.yaml Outdated Show resolved Hide resolved
@fatsheep9146 fatsheep9146 added the ready to merge Code review completed; ready to merge by maintainers label Nov 26, 2024
@andrzej-stencel andrzej-stencel merged commit 6c43968 into open-telemetry:main Nov 27, 2024
168 checks passed
@github-actions github-actions bot added this to the next release milestone Nov 27, 2024
shivanthzen pushed a commit to shivanthzen/opentelemetry-collector-contrib that referenced this pull request Dec 5, 2024
…ation (open-telemetry#36148)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
The changes includes providing a custom `DecoderConfig` with
`ErrorUnused` enabled instead of using the default `DecoderConfig`
provided by `koanf`

<!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. -->
#### Link to tracking issue
Fixes: open-telemetry#35838 

<!--Describe what testing was performed and which tests were added.-->

<!--Describe the documentation added.-->

<!--Please delete paragraphs that you did not use before submitting.-->

---------

Signed-off-by: Alok Kumar Singh <[email protected]>
Co-authored-by: Evan Bradley <[email protected]>
ZenoCC-Peng pushed a commit to ZenoCC-Peng/opentelemetry-collector-contrib that referenced this pull request Dec 6, 2024
…ation (open-telemetry#36148)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
The changes includes providing a custom `DecoderConfig` with
`ErrorUnused` enabled instead of using the default `DecoderConfig`
provided by `koanf`

<!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. -->
#### Link to tracking issue
Fixes: open-telemetry#35838 

<!--Describe what testing was performed and which tests were added.-->

<!--Describe the documentation added.-->

<!--Please delete paragraphs that you did not use before submitting.-->

---------

Signed-off-by: Alok Kumar Singh <[email protected]>
Co-authored-by: Evan Bradley <[email protected]>
sbylica-splunk pushed a commit to sbylica-splunk/opentelemetry-collector-contrib that referenced this pull request Dec 17, 2024
…ation (open-telemetry#36148)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
The changes includes providing a custom `DecoderConfig` with
`ErrorUnused` enabled instead of using the default `DecoderConfig`
provided by `koanf`

<!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. -->
#### Link to tracking issue
Fixes: open-telemetry#35838 

<!--Describe what testing was performed and which tests were added.-->

<!--Describe the documentation added.-->

<!--Please delete paragraphs that you did not use before submitting.-->

---------

Signed-off-by: Alok Kumar Singh <[email protected]>
Co-authored-by: Evan Bradley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmd/opampsupervisor ready to merge Code review completed; ready to merge by maintainers receiver/activedirectoryds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[cmd/opampsupervisor] Strictly unmarshal the supervisor configuration
5 participants