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

Support custom basepath in HotROD #1894

Merged
merged 12 commits into from
Dec 16, 2019

Conversation

jan25
Copy link
Contributor

@jan25 jan25 commented Nov 3, 2019

Which problem is this PR solving?

Short description of the changes

  • Added a new basepath argument to cmd/root.go
  • Adjusted index.html to use relative path for ajax requests

TODO

  • May need to add this to the docs somewhere?

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>
Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>
Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>
@codecov
Copy link

codecov bot commented Nov 3, 2019

Codecov Report

Merging #1894 into master will decrease coverage by 1.48%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1894      +/-   ##
==========================================
- Coverage   98.45%   96.97%   -1.49%     
==========================================
  Files         198      203       +5     
  Lines        9740    10062     +322     
==========================================
+ Hits         9590     9758     +168     
- Misses        114      266     +152     
- Partials       36       38       +2
Impacted Files Coverage Δ
cmd/agent/app/reporter/flags.go 78.57% <0%> (-21.43%) ⬇️
...gin/storage/cassandra/spanstore/operation_names.go 97.59% <0%> (-2.41%) ⬇️
...lugin/sampling/strategystore/adaptive/processor.go 99.2% <0%> (-0.8%) ⬇️
plugin/storage/cassandra/spanstore/reader.go 100% <0%> (ø) ⬆️
cmd/collector/app/builder/span_handler_builder.go 100% <0%> (ø) ⬆️
plugin/storage/es/spanstore/dbmodel/from_domain.go 100% <0%> (ø) ⬆️
storage/spanstore/metrics/decorator.go 100% <0%> (ø) ⬆️
cmd/query/app/http_handler.go 100% <0%> (ø) ⬆️
cmd/collector/app/span_processor.go 100% <0%> (ø) ⬆️
cmd/collector/app/options.go 100% <0%> (ø) ⬆️
... and 21 more

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 0cc56c5...d505157. Read the comment docs.

Copy link
Contributor

@albertteoh albertteoh left a comment

Choose a reason for hiding this comment

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

Looks pretty good, I have a few comments/questions about simplifying the implementation and making it a bit more robust.

examples/hotrod/services/frontend/server.go Outdated Show resolved Hide resolved
examples/hotrod/services/frontend/server.go Outdated Show resolved Hide resolved
examples/hotrod/cmd/root.go Outdated Show resolved Hide resolved
@jan25
Copy link
Contributor Author

jan25 commented Nov 14, 2019

@albertteoh Thanks for the review! I like your idea about using path library, I'll make those changes.

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>
@jan25
Copy link
Contributor Author

jan25 commented Nov 14, 2019

Hi @pavolloffay @yurishkuro! could you please review this? This is a simple change :)

Copy link
Contributor

@albertteoh albertteoh left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @jan25!

@pavolloffay pavolloffay changed the title [HotROD] Support for custom basepath Support for custom basepath in HotROD Dec 13, 2019
Copy link
Member

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

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

LGTM just one minor comment

@@ -73,6 +75,9 @@ func init() {
RootCmd.PersistentFlags().IntVarP(&frontendPort, "frontend-service-port", "f", 8080, "Port for frontend service")
RootCmd.PersistentFlags().IntVarP(&routePort, "route-service-port", "r", 8083, "Port for routing service")

// Flag for serving frontend at custom basepath url
RootCmd.PersistentFlags().StringVarP(&basepath, "basepath", "b", "", "Basepath for frontend service")
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add a default value / it makes the help command more self explanatory.

  -b, --basepath string                  Basepath for frontend service (default "/")

instead of

  -b, --basepath string                  Basepath for frontend service

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>
@pavolloffay pavolloffay changed the title Support for custom basepath in HotROD Support custom basepath in HotROD Dec 16, 2019
@pavolloffay pavolloffay merged commit 65ba726 into jaegertracing:master Dec 16, 2019
@jan25 jan25 deleted the hotrod-basepath branch December 16, 2019 16:41
@pavolloffay pavolloffay added this to the Release 1.16 milestone Dec 17, 2019
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.

[HotROD] Add a base path option
3 participants