-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
Added Micronaut configuration points #15005
Conversation
thanks for the PR. please update the samples when you've time: |
Sorry, I was sure I'd done that but guess not. |
@Client( | ||
{{#configureClientId}}id = "{{clientId}}",{{/configureClientId}} | ||
path = "${{openbrace}}{{{applicationName}}}{{basePathSeparator}}base-path{{closebrace}}" | ||
) |
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.
Could you change it to
@Client({{#configureClientId}}
id = "{{clientId}}",{{/configureClientId}}
path = "${{openbrace}}{{{applicationName}}}{{basePathSeparator}}base-path{{closebrace}}"
)
so that there wouldn't be an extra line in the generated code?
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.
@thrykol Is there anyway of omitting the path? I'd like to specify a client id but keep the path in my application.yml
.
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.
The path is a property in the application.yml.
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.
What if you don't want to make use of the generated yml? I only want to generate the declarative client and specify the client id.
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.
Hmm, yes I suppose it is better to set the URL with micronaut.http.services.{id}.url
if the @Client(id=)
is used. This supports all kinds of use cases.
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.
What would be the best way to achieve this @andriy-dmytruk? Adding an includePath
config option which defaults to true
?
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.
Probably removing the path all along and simply make it configurable in the config by the client id would be the best.
Otherwise, everything looks good to me |
It would seem that |
@thrykol can you please take a look at the circleci failure when you've time? https://app.circleci.com/pipelines/github/OpenAPITools/openapi-generator/22682/workflows/1b970176-8ecd-4498-81f7-8f49c3ccecc1/jobs/63308 it seems to be related to this change. |
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.
Looks good, thanks.
@wing328 Can you merge?
@bbdouglas @sreeshas @jfiala @lukoyanov @cbornet @jeff9finger @karismann @Zomzog @lwlee2608
PR checklist
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*
.For Windows users, please run the script in Git BASH.
master
(6.3.0) (minor release - breaking changes with fallbacks),7.0.x
(breaking changes without fallbacks)