-
Notifications
You must be signed in to change notification settings - Fork 6k
Fix Issue 7924 - Allow host and base path to be changed independently #7996
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
Conversation
Tests seem to pass, so not sure what I've done to make the CI fail now... Is there anything else I need to do to move this forward? (not sure who to tag, @jeff9finger?)
jeff9finger
commented
Apr 16, 2018
I like the concept. However, I think this would be more useful to provide more granularity here. Combining the schema, host and port into a property named "host" is a bit misleading. I'd like to see separate properties for schema, host, and port (or at least schema and host with the port combined with the host). An alternative would be to rename the property to something besides "host".
What does the community think?
Was erring on the side of making small changes, but agree it would make sense to split out the scheme as well and am happy to do that 🙂. (Alternatively, if there's already some precedent for this in swagger-codegen, then I'm also happy to follow that.)
Separated out the scheme, let me know if there's anything else you want from me.
@jeff9finger
jeff9finger
Apr 18, 2018
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this provide a default for "http" in a similar way that host does with "localhost"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand how this works correctly, then I believe this defaults to https if a scheme isn't specified (https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/main/java/io/swagger/codegen/DefaultGenerator.java#L98).
@jeff9finger
jeff9finger
Apr 18, 2018
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. that is correct. Thanks for pointing that out.
jeff9finger
commented
Apr 18, 2018
Also, this seems useful for all the java client libraries. Would you consider adding it to the other Java client libraries?
My project happens to use this library so that's what I've been testing with. I'd rather not edit the other Java client libraries since there are at least some where it looks my change won't easily apply, and I don't want to touch more code that I don't have time to test right now!
@jeff9finger
jeff9finger
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
jeff9finger
commented
Apr 18, 2018
@jeff9finger Do you know if this is going to be merged soon, or are you planning to wait for the other libraries to be updated?
jeff9finger
commented
Apr 24, 2018
@wing328 Can we go ahead and get this merged?
@wing328 bump?
callmeberzerker
commented
Nov 2, 2018
Any updates?
4ba5164 to
949f0e6
Compare
PR checklist
./bin/to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.shand./bin/security/{LANG}-petstore.shif updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in.\bin\windows\.3.0.0branch for changes related to OpenAPI spec 3.0. Default:master.@bbdouglas @JFCote @sreeshas @jfiala @lukoyanov @cbornet @jeff9finger
Description of the PR
Fixes #7924
Separate the hostname and the base path so that they can be changed independently of one another