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

Normalize CLI flags to use host:port addresses #2212

Merged
merged 13 commits into from
May 19, 2020
Merged

Normalize CLI flags to use host:port addresses #2212

merged 13 commits into from
May 19, 2020

Conversation

ohdearaugustin
Copy link
Contributor

Signed-off-by: Jonas Reindl [email protected]

Which problem is this PR solving?

Short description of the changes

  • Normalize CLI flags to use host:port address format
  • Add HostPort Flag to query app as it was left out

@ohdearaugustin ohdearaugustin requested a review from a team as a code owner May 2, 2020 18:03
@ohdearaugustin ohdearaugustin requested a review from pavolloffay May 2, 2020 18:03
@ohdearaugustin ohdearaugustin force-pushed the normalize-port-flags branch from 012504f to deb2d70 Compare May 2, 2020 18:04
yurishkuro
yurishkuro reviewed May 3, 2020
View reviewed changes
Copy link
Member

@yurishkuro yurishkuro 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 PR. Please make sure the CI is green, there are some tests that are failing.

cmd/query/app/flags.go Outdated Show resolved Hide resolved
cmd/query/app/flags.go Outdated Show resolved Hide resolved
cmd/query/app/flags.go Outdated Show resolved Hide resolved
cmd/query/app/flags.go Outdated Show resolved Hide resolved
cmd/query/app/flags_test.go Show resolved Hide resolved
cmd/query/app/flags_test.go Outdated Show resolved Hide resolved
cmd/query/app/server.go Outdated Show resolved Hide resolved
cmd/query/app/server.go Show resolved Hide resolved
cmd/query/app/server_test.go Outdated Show resolved Hide resolved
cmd/query/app/ui/placeholder/gen_assets.go Outdated Show resolved Hide resolved
@ohdearaugustin ohdearaugustin force-pushed the normalize-port-flags branch 2 times, most recently from 07de03e to bc3e573 Compare May 15, 2020 21:00
ohdearaugustin added 8 commits May 15, 2020 23:14
Normalize CLI flags to use host:port addresses
d04f1c2 
Signed-off-by: ohdearaugustin <[email protected]>
revert change to master
72091f4 
Signed-off-by: ohdearaugustin <[email protected]>
refactor getAddressFromCLIOptions
3946dd6 
Signed-off-by: ohdearaugustin <[email protected]>
cleanup flags
184e264 
Signed-off-by: ohdearaugustin <[email protected]>
add GetAddressFromCLIOptionHostPort test
1d3319e 
Signed-off-by: ohdearaugustin <[email protected]>
add port to logging and fix tests
7e5685f 
Signed-off-by: ohdearaugustin <[email protected]>
cleanup tests
2697e68 
Signed-off-by: ohdearaugustin <[email protected]>
remove comment
21dfa9e 
Signed-off-by: ohdearaugustin <[email protected]>
@ohdearaugustin ohdearaugustin force-pushed the normalize-port-flags branch from 34440c0 to 21dfa9e Compare May 15, 2020 21:14
@codecov
Copy link

codecov bot commented May 15, 2020 •
edited
Loading

Codecov Report

Merging #2212 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2212      +/-   ##
==========================================
+ Coverage   96.15%   96.18%   +0.02%     
==========================================
  Files         219      219              
  Lines       10650    10652       +2     
==========================================
+ Hits        10241    10246       +5     
+ Misses        353      351       -2     
+ Partials       56       55       -1
Impacted Files Coverage Δ
cmd/collector/app/builder_flags.go 100.00% <100.00%> (ø)
cmd/query/app/flags.go 100.00% <100.00%> (ø)
cmd/query/app/server.go 94.59% <100.00%> (+0.07%) ⬆️
ports/ports.go 100.00% <100.00%> (ø)
plugin/storage/badger/spanstore/reader.go 96.79% <0.00%> (ø)
...lugin/sampling/strategystore/adaptive/processor.go 100.00% <0.00%> (+0.79%) ⬆️
cmd/flags/admin.go 79.36% <0.00%> (+1.58%) ⬆️

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 2f734ec...b72d0c2. Read the comment docs.

yurishkuro
yurishkuro reviewed May 15, 2020
View reviewed changes
cmd/query/app/flags.go Outdated Show resolved Hide resolved
ohdearaugustin added 3 commits May 16, 2020 01:04
add test for port only
2cbc2b1 
Signed-off-by: ohdearaugustin <[email protected]>
fix deprecation message
47a8d49 
Signed-off-by: ohdearaugustin <[email protected]>
remove port option
054c1e5 
Signed-off-by: ohdearaugustin <[email protected]>
@ohdearaugustin ohdearaugustin force-pushed the normalize-port-flags branch from 690100c to 054c1e5 Compare May 15, 2020 23:17
yurishkuro
yurishkuro approved these changes May 19, 2020
View reviewed changes
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

Thank you!

@yurishkuro yurishkuro merged commit e46f873 into jaegertracing:master May 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurishkuro yurishkuro yurishkuro approved these changes

@pavolloffay pavolloffay Awaiting requested review from pavolloffay

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

Successfully merging this pull request may close these issues.

Normalize all CLI flags for port numbers
2 participants
@ohdearaugustin @yurishkuro