[CDF-27549] Dune App resource (Files API + zip, alpha flag)#2777
[CDF-27549] Dune App resource (Files API + zip, alpha flag)#2777
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive support for 'App' resources, specifically 'Dune apps', which are deployed by zipping application directories and uploading them as files. This involves adding new builder, CRUD, API, and YAML classes, along with updates to feature flags and testing infrastructure. The review comments highlight two main areas for improvement: the AppBuilder's copy_app_directory_to_build method should be refactored to use the AppsYAML Pydantic model for type safety and earlier validation, and the _toolkit_hash_key constant in AppCRUD should be renamed to _TOOLKIT_HASH_KEY to adhere to the UPPER_SNAKE_CASE naming convention.
| raw_content = source_file.loaded | ||
| if raw_content is None: | ||
| raise ToolkitValueError("App source file should be a YAML file.") | ||
| raw_apps = raw_content if isinstance(raw_content, list) else [raw_content] | ||
| warnings = WarningList[FileReadWarning]() | ||
| for raw_app in raw_apps: | ||
| app_external_id = raw_app.get("appExternalId") | ||
| if not app_external_id: | ||
| warnings.append( | ||
| HighSeverityWarning( | ||
| f"App in {source_file.source.path.as_posix()!r} has no appExternalId defined. " | ||
| f"This is used to match the app to the app directory.", | ||
| ), | ||
| ) | ||
| continue | ||
| if not raw_app.get("version"): | ||
| warnings.append( | ||
| HighSeverityWarning( | ||
| f"App {app_external_id} in {source_file.source.path.as_posix()!r} has no version defined.", | ||
| ), | ||
| ) | ||
|
|
||
| app_directory = source_file.source.path.with_name(app_external_id) | ||
|
|
||
| if not app_directory.is_dir(): | ||
| raise ToolkitNotADirectoryError( | ||
| f"App directory not found for appExternalId {app_external_id} defined in {source_file.source.path.as_posix()!r}.", | ||
| ) | ||
|
|
||
| destination = self.build_dir / self.resource_folder / app_external_id | ||
| if destination.exists(): | ||
| raise ToolkitFileExistsError( | ||
| f"App {app_external_id!r} is duplicated. If this is unexpected, ensure you have a clean build directory.", | ||
| ) | ||
| shutil.copytree(app_directory, destination, ignore=shutil.ignore_patterns("__pycache__")) | ||
|
|
||
| return warnings |
There was a problem hiding this comment.
The current implementation uses dict.get() on raw dictionaries, which goes against the repository's style guide that promotes using typed data structures for safety and clarity (lines 6-7, 41). Refactoring to use the AppsYAML Pydantic model for validation will make the code more robust and align with best practices.
This also fixes a potential bug where the build process would continue even if a version is missing, only to fail later during deployment. With this change, the validation happens earlier.
You'll need to add the following imports:
from pydantic import ValidationError
from cognite_toolkit._cdf_tk.yaml_classes import AppsYAML raw_content = source_file.loaded
if raw_content is None:
raise ToolkitValueError("App source file should be a YAML file.")
raw_apps = raw_content if isinstance(raw_content, list) else [raw_content]
warnings = WarningList[FileReadWarning]()
for raw_app in raw_apps:
try:
app_config = AppsYAML.model_validate(raw_app)
except ValidationError as e:
warnings.append(
HighSeverityWarning(
f"App in {source_file.source.path.as_posix()!r} has invalid configuration: {e}"
)
)
continue
app_directory = source_file.source.path.with_name(app_config.app_external_id)
if not app_directory.is_dir():
raise ToolkitNotADirectoryError(
f"App directory not found for appExternalId {app_config.app_external_id} defined in {source_file.source.path.as_posix()!r}.",
)
destination = self.build_dir / self.resource_folder / app_config.app_external_id
if destination.exists():
raise ToolkitFileExistsError(
f"App {app_config.app_external_id!r} is duplicated. If this is unexpected, ensure you have a clean build directory.",
)
shutil.copytree(app_directory, destination, ignore=shutil.ignore_patterns("__pycache__"))
return warningsReferences
- Code should use dataclasses or Pydantic models for complex data structures instead of untyped dictionaries to ensure type safety. (link)
| _doc_url = "Files/operation/initFileUpload" | ||
| metadata_value_limit = 512 | ||
| support_update = True | ||
| _toolkit_hash_key = "cdf-toolkit-app-hash" |
There was a problem hiding this comment.
According to the style guide (line 95), constants should be in UPPER_SNAKE_CASE. Please rename _toolkit_hash_key to _TOOLKIT_HASH_KEY and update its usage on line 114.
| _toolkit_hash_key = "cdf-toolkit-app-hash" | |
| _TOOLKIT_HASH_KEY = "cdf-toolkit-app-hash" |
References
- Constants should be named using
UPPER_SNAKE_CASE. (link)
Apps live under apps/ with kind App: zip sources like functions, upload via Files API init to /dune-apps/ and PUT zip (Streamlit-style flow). Gated by hidden alpha flag apps in cdf.toml.
ronpal
force-pushed
the
cdf-27549-dune-apps
branch
from
5876d2b to
c1b5e43
Compare
☂️ Python Coverage
Overall Coverage
New Files
Modified Files
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2777 +/- ##
==========================================
- Coverage 85.43% 85.05% -0.38%
==========================================
Files 447 452 +5
Lines 38792 39135 +343
==========================================
+ Hits 33141 33286 +145
- Misses 5651 5849 +198
🚀 New features to boost your workflow:
|
- Merge main: use tool.filemetadata.upload_content for zip PUT (raises on failure). - AppBuilder: validate app entries with AppsYAML instead of raw dict access. - AppCRUD: rename hash metadata key constant to _TOOLKIT_HASH_KEY. - FileMetadataAPI.upload_content: mirror upload_file (get_success_or_raise); return None.
1 participant
Description
Adds a Toolkit resource for Dune apps: folder
apps/, kindApp, behind the hidden alpha flagappsincdf.toml.Deployment mirrors Streamlit (Files API:
POST /fileswith directory/dune-apps/, thenPUTtouploadUrl) while packaging sources as a zip from the app directory (same zipping idea as functions). File external id is<appExternalId>-<version>; metadata matches the Dune upload shape (published,name,description,externalId,version, plusappExternalIdfor round-trip).Bump
Changelog
Added
AppCRUD,AppBuilder,AppsYAML,AppsAPI(tool.apps),DuneAppFilter, andFlags.APPS(hidden).