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/builder] Update builder flags precedence #5726

Merged
merged 7 commits into from
Jul 26, 2022

Conversation

TylerHelmuth
Copy link
Member

Description:
This PR updates the builder's config initialization so that flags always take precedence over configuration files or default values.

I opted not to use the collector's resolvers/converter approach as it felt overkill for this command. This approach takes advantage of the command's existing use of global vars, which isn't the best, but is at least consistent.

Link to tracking Issue:
Fixes #5704

Testing:
Tested locally

Documentation:
I didn't update the README because it feels like flags taking precedence is the norm and the flags are already called out.

@TylerHelmuth TylerHelmuth requested review from a team and jpkrohling July 20, 2022 21:57
@codecov
Copy link

codecov bot commented Jul 20, 2022

Codecov Report

Merging #5726 (968e247) into main (9930104) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #5726   +/-   ##
=======================================
  Coverage   91.56%   91.56%           
=======================================
  Files         192      192           
  Lines       11411    11411           
=======================================
  Hits        10449    10449           
  Misses        768      768           
  Partials      194      194           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9930104...968e247. Read the comment docs.

@TylerHelmuth
Copy link
Member Author

There is a defect with this solution, closing until I sort it out.

@TylerHelmuth TylerHelmuth reopened this Jul 20, 2022
@bogdandrutu
Copy link
Member

Please rebase to resolve conflicts.

@TylerHelmuth
Copy link
Member Author

Please rebase to resolve conflicts.

done

@codeboten codeboten merged commit 76698c1 into open-telemetry:main Jul 26, 2022
@TylerHelmuth TylerHelmuth deleted the issue-5704 branch July 26, 2022 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

builder flags do not override configuration values
4 participants