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

Update koanf to v2 #7997

Merged
merged 3 commits into from
Jun 29, 2023
Merged

Update koanf to v2 #7997

merged 3 commits into from
Jun 29, 2023

Conversation

dbason
Copy link
Contributor

@dbason dbason commented Jun 28, 2023

This PR updates the koanf dependency to v2

@dbason dbason requested a review from a team as a code owner June 28, 2023 17:28
@dbason dbason requested a review from jpkrohling June 28, 2023 17:28
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Is this addressing an issue or limitation with the older koanf library?

@mx-psi
Copy link
Member

mx-psi commented Jun 29, 2023

@knadh If you have the time, can you help us understand what changed between koanf v1 and v2 and what would be the benefits and changes if we update?

@knadh
Copy link

knadh commented Jun 29, 2023

Hi @mx-psi. Functionally, no changes. The package structure has changed where every provider/parser has become its own module (with its own go.mod) to become a one repository-many modules system.

Benefits:
This removes references to all indirect dependencies in the main go.mod significantly de-cluttering it. These would never be compiled into the build, but go get .../koanf would still pull them in the dev environment in v1. Check out the main package's go.mod in v2.

It is safe (and also easy) to upgrade to v2 as it is functionally identical to v1. While this needn't be done on priority, I would recommend moving to v2 as that is where any continued improvements will land. v1 is frozen and will only receive security updates.

@mx-psi
Copy link
Member

mx-psi commented Jun 29, 2023

Thanks for the detailed explanation @knadh :) If it helps reduce our dependency footprint and it doesn't imply any user-facing changes I am all in for moving to v2. What do you think @codeboten ?

Copy link
Contributor

@codeboten codeboten 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 the explanation @knadh!

@codeboten codeboten added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jun 29, 2023
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Please run make gotidy to tidy up the dependencies

@dbason
Copy link
Contributor Author

dbason commented Jun 29, 2023

I suspect the tests failed due to the github outage.

@codecov
Copy link

codecov bot commented Jun 29, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (f990e99) 90.96% compared to head (a6d870a) 90.96%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7997   +/-   ##
=======================================
  Coverage   90.96%   90.96%           
=======================================
  Files         300      300           
  Lines       15090    15090           
=======================================
  Hits        13726    13726           
  Misses       1091     1091           
  Partials      273      273           
Impacted Files Coverage Δ
cmd/builder/internal/command.go 62.50% <ø> (ø)
cmd/builder/internal/config/default.go 100.00% <ø> (ø)
confmap/confmap.go 90.12% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@codeboten
Copy link
Contributor

Needs one more make crosslink && make gotidy... not sure why crosslink included a bunch of imports in a previous commit

@codeboten codeboten mentioned this pull request Jun 29, 2023
@dbason
Copy link
Contributor Author

dbason commented Jun 29, 2023

I've done that now and squashed into the last commit

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Thanks it's looking good now

@codeboten codeboten merged commit 50c94c9 into open-telemetry:main Jun 29, 2023
30 checks passed
@github-actions github-actions bot added this to the next release milestone Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants