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 #1827

Merged
merged 21 commits into from
Mar 31, 2020
Merged

Normalize CLI flags to use host:port addresses #1827

merged 21 commits into from
Mar 31, 2020

Conversation

annanay25
Copy link
Member

@annanay25 annanay25 commented Oct 2, 2019 •
edited
Loading

Signed-off-by: Annanay [email protected]

Which problem is this PR solving?

Short description of the changes

  • Normalize CLI flags to use host:port address format

yurishkuro
yurishkuro reviewed Oct 2, 2019
View reviewed changes
cmd/all-in-one/main.go Outdated Show resolved Hide resolved
cmd/collector/app/builder/builder_flags.go Outdated Show resolved Hide resolved
cmd/collector/app/builder/builder_flags.go Outdated Show resolved Hide resolved
cmd/collector/app/builder/builder_flags.go Outdated Show resolved Hide resolved
ports/ports.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 3, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@b1868a6). Click here to learn what that means.
The diff coverage is 73.46%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1827   +/-   ##
=========================================
  Coverage          ?   96.14%           
=========================================
  Files             ?      219           
  Lines             ?    10585           
  Branches          ?        0           
=========================================
  Hits              ?    10177           
  Misses            ?      353           
  Partials          ?       55
Impacted Files Coverage Δ
cmd/flags/service.go 0% <0%> (ø)
cmd/collector/app/server/zipkin.go 0% <0%> (ø)
ports/ports.go 0% <0%> (ø)
cmd/collector/app/server/http.go 0% <0%> (ø)
cmd/collector/app/collector.go 69.11% <100%> (ø)
cmd/collector/app/server/grpc.go 65.38% <100%> (ø)
cmd/collector/app/builder_flags.go 100% <100%> (ø)
cmd/flags/admin.go 79.36% <83.33%> (ø)

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 b1868a6...0717a8d. Read the comment docs.

yurishkuro
yurishkuro reviewed Oct 3, 2019
View reviewed changes
cmd/all-in-one/main.go Outdated Show resolved Hide resolved
@pavolloffay
Copy link
Member

@annanay25 could you please document this PR in changelog? Especially if there are breaking changes.

yurishkuro
yurishkuro reviewed Oct 21, 2019
View reviewed changes
cmd/flags/admin.go Outdated Show resolved Hide resolved
@objectiser objectiser mentioned this pull request Oct 28, 2019
Merged
Checkpoint
d3e4385 
Signed-off-by: Annanay Agarwal <[email protected]>
yurishkuro
yurishkuro reviewed Mar 10, 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.

@annanay25 do you plan to finish this PR?

cmd/flags/admin.go Outdated Show resolved Hide resolved
@annanay25
Copy link
Member Author

Hi @yurishkuro; thanks for the ping, I'll try and wrap it up this week. :)

@annanay25 annanay25 requested a review from a team as a code owner March 13, 2020 11:56
yurishkuro
yurishkuro reviewed Mar 13, 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.

Please use deprecation message suggested in #2113.

We also need to update the documentation and READMEs where we give examples of start commands.

CHANGELOG.md Outdated Show resolved Hide resolved
cmd/collector/app/builder_flags.go Outdated Show resolved Hide resolved
cmd/collector/app/builder_flags.go Outdated Show resolved Hide resolved
cmd/collector/app/builder_flags.go Outdated Show resolved Hide resolved
cmd/flags/admin.go Show resolved Hide resolved
cmd/flags/admin.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
yurishkuro
yurishkuro approved these changes Mar 30, 2020
View reviewed changes
cmd/flags/admin.go Outdated Show resolved Hide resolved
yurishkuro
yurishkuro reviewed Mar 30, 2020
View reviewed changes
go.mod Outdated Show resolved Hide resolved
@yurishkuro
Copy link
Member

build error: Go fmt, license check, or import ordering failures, run 'make fmt'

@annanay25
Copy link
Member Author

/re-run-ci

@yurishkuro
Copy link
Member

trying to restart CI

@yurishkuro yurishkuro closed this Mar 31, 2020
@yurishkuro yurishkuro reopened this Mar 31, 2020
Yuri Shkuro added 2 commits March 31, 2020 13:37
Add small test
d1e14af 
Signed-off-by: Yuri Shkuro <[email protected]>
Another silly test
0717a8d 
Signed-off-by: Yuri Shkuro <[email protected]>
@yurishkuro yurishkuro merged commit 87b1910 into jaegertracing:master Mar 31, 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

@black-adder black-adder Awaiting requested review from black-adder

@jpkrohling jpkrohling Awaiting requested review from jpkrohling

@objectiser objectiser Awaiting requested review from objectiser

@pavolloffay pavolloffay Awaiting requested review from pavolloffay

@tiffon tiffon Awaiting requested review from tiffon

@vprithvi vprithvi Awaiting requested review from vprithvi

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
3 participants
@annanay25 @pavolloffay @yurishkuro