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

Add project skeleton and check-in snapshot of generated SDK for Azure App Configuration. #6176

Merged
merged 10 commits into from
Nov 8, 2024

Conversation

ahsonkhan
Copy link
Contributor

@ahsonkhan ahsonkhan commented Nov 5, 2024

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:

  1. Are the various CMakeLists.txt and vcpkg files correct?
  2. Is the package name, service name, and folder location as we'd expect?
  3. Any feedback on how the files are laid out on disk.
  4. Is the CI setup as we'd expect?

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:

  1. Start by copying the template project folder
  • Rename to appconfiguration, and rename sub-folder to azure-data-appconfiguration.
  • Introduce a data sub-folder and rename sub-folders.
  1. Modify the header files as needed
  • Modify ApiViewSettings.json
    • Update filterNamespace, previewName, serviceName, packageName
  • Save a top-level appconfiguration.hpp, with a client, models, and options header file
  • Rename rest_client.hpp into configuration_client.hpp and split it into the separate expected headers.
  • Modify dll_import_export.hpp and rtti.hpp
    • Find/Replace _TEMPLATE_ with _DATA_APPCONFIGURATION_ (in both files)
    • Remove unnecessary namespace in dll_import_export.
  • Remove template_client.hpp
  • Update appconfiguration.hpp root header comment.
  • Add doc comments [TODO] - stop gap in place to disable docs generation for this target in CI - [EngSys] Opt outs for faster package iteration #6187
  1. Modify the source files as needed
  • Modify package_version.hpp namespace
  • Rename rest_client.cpp to appconfiguration_client.cpp and update #include headers
  • Remove template_client.cpp
  • Update HTTP pipeline ctor to include package version
  • Save package telemetry name to a shared known constant (_detail::AppConfigurationServicePackageName), to be re-used by multiple clients.
  • Split out LROs, PagedResponse, and other implementations into separate source files. [TBD]
  • Fix nesting issue with locals being over-written.
  1. Update repo root CMakeLists.txt (add_subdirectory(sdk/appconfiguration))
  2. Update SDK root CMakeLists.txt (modify project name)
  3. Update package root CMakeLists.txt
  • Modify the template to remove/add comment and remove duplicative "include" from template
  • Add AZ_ALL_LIBRARIES if/else
  • Attestation is missing include(CreateMapFile)
  • Missing detail about FETCH_SOURCE_DEPS [TODO]
  • Modify header and source sets
  • Missing BUILD_SAMPLES [TODO]
  • Modify test/CMakeLists.txt to be under test/ut.
  1. Modify ci.yml
  • Added Line and Branch coverage targets of 0 to start with.
  • What do you put for CtestRegex?
  • Should it be batch: true
  1. Modify vcpkg.json file at root of package (the template is outdated)
  • What do we use here? version-semver? vs version-string?
  • There is inconsistency here, is it even needed?
  1. Update NOTICE.txt (rename template to data-appconfiguration)
  2. Modify Changelog
  • Simplify it to the first version, unreleased, with the required headings
  1. cgmanifest [no action]
  • Unlike all other SDKs, attestation uses different version of doxygen (1.8.18) from azuresdktooling blob.
  1. Modify root README [TODO]
  2. Modify vcpkg folder
  • Modify Config.cmake.in (renames)
  • Modify portfile.cmake (renames)
  • Modify vcpkg.json (renames + update core dependency to latest)
  1. Modify the test/ut folder and CMakeLists.txt
  • Rename azure-template to azure-data-appconfiguration in test CMakeLists.txt
  • Link identity and other gtest libraries in test CMakeLists.txt

@ahsonkhan ahsonkhan added the App Configuration Azure.ApplicationModel.Configuration label Nov 5, 2024
@ahsonkhan ahsonkhan self-assigned this Nov 5, 2024
Copy link
Member

@LarryOsterman LarryOsterman left a 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.

@ahsonkhan
Copy link
Contributor Author

ahsonkhan commented Nov 5, 2024

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.

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.

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.

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 client.tsp:
#6183

@ahsonkhan
Copy link
Contributor Author

ahsonkhan commented Nov 7, 2024

@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.

@azure-sdk
Copy link
Collaborator

API change check

APIView has identified API level changes in this PR and created following API reviews.

azure-data-appconfiguration-cpp

Copy link
Member

@danieljurek danieljurek left a 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

@ahsonkhan ahsonkhan merged commit d46cdad into Azure:main Nov 8, 2024
79 checks passed
@ahsonkhan ahsonkhan deleted the AppConfigPackageTemplate branch November 8, 2024 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
App Configuration Azure.ApplicationModel.Configuration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants