-
Notifications
You must be signed in to change notification settings - Fork 129
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
Add project skeleton and check-in snapshot of generated SDK for Azure App Configuration. #6176
Conversation
… App Configuration.
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.
There are a lot of comments here, but in general this looks reasonable.
I believe that the vast majority of them apply to codegen and are not a result of actual developer work, so they should be taken as feedback on codegen.
Several of the elements of feedback (the use of the "Key" structure for example) are elements that may be in the appconfiguration typespec, but it is worth considering renaming them in customizations since they are REALLY generic.
I'm also concerned about the use of string.empty() as a marker to indicate that a value is not supplied - I'm not sure that is always safe
I do have one substantive request though: Please do not mark this package as GA - I'm 99% sure that we're not comfortable with our codegen to release a package directly without a beta.
...uration/azure-data-appconfiguration/inc/azure/data/appconfiguration/configuration_client.hpp
Show resolved
Hide resolved
.../azure-data-appconfiguration/inc/azure/data/appconfiguration/configuration_client_models.hpp
Show resolved
Hide resolved
.../azure-data-appconfiguration/inc/azure/data/appconfiguration/configuration_client_models.hpp
Show resolved
Hide resolved
.../azure-data-appconfiguration/inc/azure/data/appconfiguration/configuration_client_models.hpp
Show resolved
Hide resolved
sdk/appconfiguration/azure-data-appconfiguration/src/configuration_client.cpp
Show resolved
Hide resolved
sdk/appconfiguration/azure-data-appconfiguration/src/configuration_client.cpp
Show resolved
Hide resolved
sdk/appconfiguration/azure-data-appconfiguration/src/configuration_client.cpp
Outdated
Show resolved
Hide resolved
sdk/appconfiguration/azure-data-appconfiguration/src/private/package_version.hpp
Outdated
Show resolved
Hide resolved
Thank you for sharing the feedback! Some of these are known codegen bugs or limitations. I will turn the others into tracking work items. Most of these are valid questions, and I believe more questions and feedback will come up as we iterate through the specifics. My next steps are to use hero scenario samples to drive the targeted discussion.
100% agreed. That is part of the plan for next steps, which is to identify where the generated models, as specified, don't work, and override them with C++ specific customizations within the |
@LarryOsterman I have addressed your blocking feedback and filed work items for the rest of the codegen/typespec/guidelines related issues. Do you have any other blocking feedback? We'll be reviewing the API surface in more depth via an APIView, in the near future, using samples to drive models/method changes, and iterate. For now, I would like to get this snapshot checked-in to build on top of. |
API change check APIView has identified API level changes in this PR and created following API reviews. |
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.
EngSys changes are OK to keep things moving. Issue filed to address these in a more long-term appropriate manner: #6187
Part of #1010
The goal of this PR is to check-in a package skeleton, laying out files on disk as expected, and establish a baseline based on what the typespec based generator is currently emitting. It isn't expected to be perfect or beta ready, but important to check-in to iterate on top of.
From a review perspective, what I am looking for is the following:
The current API shape of the generated client is very much a WIP, and will evolve based on samples and what we expect the shape to be. Hence, it isn't valuable to review the API surface just yet, as part of this PR.
There is related work needed to be done in the emitter, and improvements to the template project folder based on the findings of this exercise. Those are outside the scope of this PR and will be tracked by separate issues.
Here are the hand-written steps taken, using the template project and the generated AppConfig client source files for bootstrapping, including known TODOs and TBD decisions:
_TEMPLATE_
with_DATA_APPCONFIGURATION_
(in both files)