RDKEMW-12279 RALF Support for EntOS - PackageManager integration#722
RDKEMW-12279 RALF Support for EntOS - PackageManager integration#722SKumarMetro wants to merge 12 commits intodevelopfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for RALF (Runtime Application Lifecycle Framework) integration with EntOS PackageManager by introducing a new failure reason enum value.
Changes:
- Added GENERAL_FAILURE enum value to the FailReason enumeration in IPackageInstaller interface
|
|
||
| enum class FailReason : uint8_t { | ||
| NONE, // XXX: Not in HLA | ||
| GENERAL_FAILURE, |
There was a problem hiding this comment.
The enum value GENERAL_FAILURE is missing the required @text annotation. According to the Enum Naming convention, enum values in interfaces marked with @JSON should include @text annotations to define their JSON RPC representation.
For example, similar patterns can be seen in:
- apis/FirmwareUpdate/IFirmwareUpdate.h:30 -
VALIDATION_FAILED = 1 /* @text VALIDATION_FAILED */ - apis/USBMassStorage/IUSBMassStorage.h:35 -
READ_ONLY = 1 /* @text READ_ONLY */
The @text annotation should be added in the format: GENERAL_FAILURE /* @text GENERAL_FAILURE */,
Documentation Auto-GeneratedDocumentation has been automatically generated for the changed plugins and committed to this PR branch. The generated documentation files have been added to the PR for review. |
docs/apis/PackageManager.md
Outdated
| | [download](#download) | Downloads a resource file for an application. | | ||
| | [getConfigForPackage](#getConfigForPackage) | getConfigForPackage | | ||
| | [getLockedInfo](#getLockedInfo) | GetLockedInfo | | ||
| | [getStorageInformation](#getStorageInformation) | GetStorageInformation | |
There was a problem hiding this comment.
The documentation for the getLockedInfo API has been removed from this file, but the corresponding method GetLockedInfo still exists in the IPackageHandler interface (IAppPackageManager.h lines 322-333). This creates an inconsistency between the header file and the documentation. Either the method should be removed from the header file, or the documentation should be retained. If getLockedInfo is deprecated in favor of getLockInfo, this should be documented clearly.
This reverts commit 0f8399c.
3f0233f to
dd2ebf8
Compare
Documentation Auto-GeneratedDocumentation has been automatically generated for the changed plugins and committed to this PR branch. The generated documentation files have been added to the PR for review. |
| | params.options.priority | bool | | | ||
| | params.options.retries | integer | | | ||
| | params.options.rateLimit | integer | | |
There was a problem hiding this comment.
The parameter descriptions for the Options struct fields are empty. According to documentation best practices and custom coding guideline 1000003, all parameters should have meaningful descriptions. Add descriptions such as:
- priority: "Indicates whether this download should be prioritized"
- retries: "Number of retry attempts if download fails"
- rateLimit: "Maximum download rate limit in bytes per second"
| | result.storageinfo.EXTERNAL | struct | | | ||
| | result.storageinfo.path | string | | | ||
| | result.storageinfo.quotaKB | string | | | ||
| | result.storageinfo.usedKB | string | | |
There was a problem hiding this comment.
The StorageInfo structure documentation doesn't match the interface definition in IPackageManager.h. According to the header file (lines 75-86), StorageInfo contains two StorageDetails structs: "apps" and "persistent", each with path, quotaKB, and usedKB fields. The documentation shows "EXTERNAL" as a field which doesn't exist in the interface. The correct structure should be:
- result.storageinfo.apps (StorageDetails)
- result.storageinfo.apps.path (string)
- result.storageinfo.apps.quotaKB (string)
- result.storageinfo.apps.usedKB (string)
- result.storageinfo.persistent (StorageDetails)
- result.storageinfo.persistent.path (string)
- result.storageinfo.persistent.quotaKB (string)
- result.storageinfo.persistent.usedKB (string)
| | result.storageinfo.EXTERNAL | struct | | | |
| | result.storageinfo.path | string | | | |
| | result.storageinfo.quotaKB | string | | | |
| | result.storageinfo.usedKB | string | | | |
| | result.storageinfo.apps | StorageDetails | | | |
| | result.storageinfo.apps.path | string | | | |
| | result.storageinfo.apps.quotaKB | string | | | |
| | result.storageinfo.apps.usedKB | string | | | |
| | result.storageinfo.persistent | StorageDetails | | | |
| | result.storageinfo.persistent.path | string | | | |
| | result.storageinfo.persistent.quotaKB | string | | | |
| | result.storageinfo.persistent.usedKB | string | | |
| | result.installedIds | IPackageKeyIterator | | | ||
| | result.installedIds[#].id | string | | | ||
| | result.installedIds[#].version | string | | |
There was a problem hiding this comment.
There's a mismatch between the parameter documentation and the example response. The parameters show "result.installedIds[#].id" and "result.installedIds[#].version", but the example response shows the result as a direct array without the "installedIds" wrapper.
Based on the IPackageManager.h interface (line 178) which returns an IPackageKeyIterator, the JSON-RPC representation typically serializes iterators as arrays. The parameter documentation should either:
- Match the example and show "result[#].id" and "result[#].version", or
- The example should wrap the array in an "installedIds" field
Consistency between parameter documentation and examples is important for API consumers.
| ## *uninstall* | ||
|
|
||
| Resume | ||
| Uninstall |
There was a problem hiding this comment.
The brief description has been changed from "Uninstalls an application" to just "Uninstall". While "Uninstalls an application" was already brief, changing it to just "Uninstall" makes it even less informative. Method descriptions should be complete sentences.
| Uninstall | |
| Uninstalls an application package. |
| | params.version | string | | | ||
| | params.version | string | Version | | ||
| | params.additionalMetadata | IKeyValueIterator | Additional Metadata | | ||
| | params.additionalMetadata[#].name | string | | |
There was a problem hiding this comment.
The KeyValue struct field naming is inconsistent between interfaces. In IAppPackageManager.h (lines 208-213), the KeyValue struct used by IPackageInstaller has "name" and "value" fields. However, in IPackageManager.h (lines 96-99), the KeyValue struct has "key" and "value" fields.
Since this documentation now references IPackageManager.h (line 5), but documents the install method from IPackageInstaller (which uses "name"), there's a naming inconsistency that should be resolved. Ensure the correct struct definition is used based on which interface is actually being implemented.
| | params.additionalMetadata[#].name | string | | | |
| | params.additionalMetadata[#].key | string | | |
| | result.result | LockInfo | | | ||
| | result.result.reason | string | | | ||
| | result.result.owner | string | | |
There was a problem hiding this comment.
The result structure documentation appears incorrect. It shows "result.result.reason" and "result.result.owner", which suggests a nested "result.result" structure. However, based on the IPackageManager.h interface (line 207), the GetLockInfo method returns a LockInfo struct directly. The documentation should be:
- result (object containing LockInfo fields)
- result.reason (string)
- result.owner (string)
Not "result.result.reason" and "result.result.owner".
| | result.result | LockInfo | | | |
| | result.result.reason | string | | | |
| | result.result.owner | string | | | |
| | result.reason | string | | | |
| | result.owner | string | | |
| ## *cancel* | ||
|
|
||
| Cancels a previously issued asynchronous request. | ||
| Cancel |
There was a problem hiding this comment.
The brief description has been changed from "Cancels a previously issued asynchronous request" to just "Cancel". This makes the documentation less descriptive and harder to understand. Method descriptions should be complete sentences that clearly explain what the method does, not just restate the method name.
| ## *install* | ||
|
|
||
| PackageState | ||
| Install |
There was a problem hiding this comment.
The brief description has been changed from "Downloads and installs an application bundle" to just "Install". This reduces the clarity of the documentation. Method descriptions should be complete sentences that clearly explain what the method does.
| Install | |
| Installs the specified package on the device. |
| # PackageManager Plugin | ||
|
|
||
| **Version: [1.0.0](https://github.com/rdkcentral/entservices-apis/tree/main/apis/PackageManager/IAppPackageManager.h)** | ||
| **Version: [1.0.0](https://github.com/rdkcentral/entservices-apis/tree/main/apis/PackageManager/IPackageManager.h)** |
There was a problem hiding this comment.
The version link has been changed from IAppPackageManager.h to IPackageManager.h. However, according to the repository's README (apis/PackageManager/README.md), IPackageManager.h is the old interface that will be deleted after transitioning to the new PackageManager, while IAppPackageManager.h contains the new interfaces (IPackageDownloader, IPackageHandler, IPackageInstaller).
The documented APIs in this file appear to be from the new interfaces in IAppPackageManager.h (e.g., download with Options struct, install with additionalMetadata), not from IPackageManager.h. The reference should point to IAppPackageManager.h instead, or the PR title should clarify that this is documenting the old IPackageManager interface.
| | params.packageId | string | Package Id | | ||
| | params.version | string | | | ||
| | params.type | string | | | ||
| | params.id | string | package id | |
There was a problem hiding this comment.
The parameter descriptions for "id" and "version" now include descriptions ("package id" and "Version"), which improves clarity. However, there's an inconsistency in capitalization: "package id" is lowercase while "Version" is capitalized. For consistency, either both should be lowercase (e.g., "package id" and "version") or both should follow a consistent capitalization pattern.
| | params.id | string | package id | | |
| | params.id | string | Package ID | |
No description provided.