Support Multiple License Products when using Unity Licensing Server #282
Conversation
Add licensingProductIds param & update services-config template
|
|
|
@coderabbitai do a full review |
|
@GabLeRoux: Understood, I will perform a full review of the PR. ✅ Actions performedFull review triggered. |
WalkthroughThe changes introduce a new input parameter, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Input
participant Docker
participant LicensingServerSetup
participant ImageEnvironmentFactory
User->>Input: Provide unityLicensingProductIds
Input->>Docker: Pass unityLicensingProductIds
Docker->>LicensingServerSetup: Setup with unityLicensingProductIds
LicensingServerSetup->>ImageEnvironmentFactory: Configure environment with UNITY_LICENSING_PRODUCT_IDS
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
src/model/licensing-server-setup.ts (2)
5-9: Add type annotations and parameter documentation.The method signature could be improved with:
- Type annotation for
unityLicensingServerparameter- JSDoc documentation explaining the purpose and format of
unityLicensingProductIdsConsider applying these changes:
+/** + * Sets up the Unity licensing server configuration + * @param unityLicensingServer The URL of the Unity licensing server + * @param actionFolder The folder containing action configuration files + * @param unityLicensingProductIds Comma-separated list of Unity license product IDs + */ public static Setup( - unityLicensingServer, + unityLicensingServer: string, actionFolder: string, unityLicensingProductIds: string, )
Line range hint
11-17: Enhance error handling and return status.The error handling could be improved to provide better feedback and status reporting.
Consider these improvements:
- public static Setup( + public static async Setup( unityLicensingServer: string, actionFolder: string, unityLicensingProductIds: string, - ) { + ): Promise<boolean> { const servicesConfigPath = `${actionFolder}/unity-config/services-config.json`; const servicesConfigPathTemplate = `${servicesConfigPath}.template`; if (!fs.existsSync(servicesConfigPathTemplate)) { - core.error(`Missing services config ${servicesConfigPathTemplate}`); - return; + core.error(`Services config template not found at: ${servicesConfigPathTemplate}`); + return false; } + try { + // ... existing code ... + return true; + } catch (error) { + core.error(`Failed to setup licensing server: ${error.message}`); + return false; + } }src/model/image-environment-factory.ts (1)
30-30: LGTM with suggestions for improvement.The addition of
UNITY_LICENSING_PRODUCT_IDSenvironment variable follows the established pattern and integrates well with the existing code.Consider adding input validation and documentation:
+ // Comma-separated list of Unity license product IDs (e.g., "UC-XXXX,UC-YYYY") { name: 'UNITY_LICENSING_PRODUCT_IDS', value: parameters.unityLicensingProductIds },Also, consider adding validation before assignment:
{ name: 'UNITY_LICENSING_PRODUCT_IDS', value: parameters.unityLicensingProductIds?.split(',') .map(id => id.trim()) .filter(id => id.match(/^UC-[A-Z0-9]+$/)) .join(',') }action.yml (1)
96-99: Consider single-line description for consistency.The multi-line description format differs from other parameters in the file. Consider consolidating it into a single line for consistency:
- description: - 'Comma separated list of license product identifiers to request licenses for from the license server. Not setting - this will request the default license product configured on the licensing server. Only applicable if - unityLicensingServer is set.' + description: 'Comma separated list of license product identifiers to request licenses for from the license server. Not setting this will request the default license product configured on the licensing server. Only applicable if unityLicensingServer is set.'src/model/docker.ts (1)
37-41: Document the new parameter in JSDoc.Add documentation for the new parameter to improve code maintainability.
Add JSDoc above the run method:
/** * Run a Docker container with the specified parameters * @param image - The Docker image to run * @param parameters - Configuration parameters * @param parameters.unityLicensingServer - Unity licensing server configuration * @param parameters.actionFolder - Action folder path * @param parameters.unityLicensingProductIds - Comma-separated list of Unity license product IDs * @param silent - Whether to suppress output */src/model/input.ts (2)
90-90: Consider adding input validation for unityLicensingProductIds.While the implementation follows the existing pattern, consider adding validation to ensure the product IDs are in the correct format (e.g., comma-separated valid identifiers) to catch configuration errors early.
Example validation:
if (unityLicensingProductIds !== '') { const productIds = unityLicensingProductIds.split(',').map(id => id.trim()); if (productIds.some(id => !/^[\w-]+$/.test(id))) { throw new Error(`Invalid unityLicensingProductIds format. Expected comma-separated alphanumeric IDs.`); } }
243-243: Add TypeScript type annotations for better type safety.Consider adding type annotations to the return object to improve type safety and documentation.
Example:
interface InputConfig { // ... existing properties unityLicensingServer: string; unityLicensingProductIds: string; // ... remaining properties } public static getFromUser(): InputConfig { // ... existing implementation }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.mapdist/unity-config/services-config.json.templateis excluded by!**/dist/**
📒 Files selected for processing (6)
- action.yml (1 hunks)
- src/main.ts (2 hunks)
- src/model/docker.ts (1 hunks)
- src/model/image-environment-factory.ts (1 hunks)
- src/model/input.ts (2 hunks)
- src/model/licensing-server-setup.ts (2 hunks)
🔇 Additional comments (6)
src/main.ts (2)
32-32: LGTM: Clean integration of the new licensing parameter.The addition of
unityLicensingProductIdsfollows the existing pattern and is positioned logically with other Unity-related parameters.
70-70: Verify the Docker configuration handling.The parameter is correctly forwarded to Docker.run(). However, let's verify that the Docker configuration properly handles this parameter.
src/model/image-environment-factory.ts (1)
Line range hint
1-93: Verify error handling in dependent components.The new environment variable affects the Docker container configuration. Ensure proper error handling exists in the Unity licensing server setup code.
Let's check the related files:
action.yml (1)
93-99: LGTM! Well-integrated licensing parameter.The new
unityLicensingProductIdsparameter is well-designed:
- Optional with empty default maintains backward compatibility
- Clear dependency on
unityLicensingServeris documented- Comma-separated format is explicitly mentioned
src/model/docker.ts (1)
37-41: Verify integration with LicensingServerSetup.The changes look good, but we should verify the integration with the LicensingServerSetup class.
Let's check the implementation of the Setup method:
✅ Verification successful
Let me verify the source implementation rather than the compiled code:
Integration with LicensingServerSetup is correctly implemented
The
Setupmethod is being called with the correct parameters in the same order as defined in the class:
unityLicensingServer- for replacing the '%URL%' placeholderactionFolder- for constructing the config pathsunityLicensingProductIds- for replacing the '%LICENSE_PRODUCT_IDS%' placeholderThe implementation handles the configuration file setup properly with appropriate error handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the LicensingServerSetup class implementation ast-grep --pattern $'class LicensingServerSetup { $$$ static Setup($$$) { $$$ } $$$ }'Length of output: 1160
Script:
#!/bin/bash # Search for the LicensingServerSetup class in TypeScript files rg -t typescript "class LicensingServerSetup" -A 15Length of output: 88
Script:
#!/bin/bash # Search for the LicensingServerSetup class in .ts files rg "class LicensingServerSetup" -g "*.ts" -A 15Length of output: 1238
src/model/input.ts (1)
90-91: Document and validate the relationship with unityLicensingServer.Since this parameter is related to the licensing server functionality, consider:
- Adding JSDoc comments to document the relationship between
unityLicensingProductIdsandunityLicensingServer- Implementing conditional validation when the licensing server is specified
Let's verify the usage pattern:
Example implementation:
/** * @param unityLicensingServer - The Unity licensing server URL * @param unityLicensingProductIds - Comma-separated list of product IDs to request from the licensing server * @throws {Error} When product IDs are specified without a licensing server */ if (unityLicensingProductIds !== '' && unityLicensingServer === '') { throw new Error('unityLicensingProductIds requires unityLicensingServer to be specified'); }
| @@ -13,6 +17,7 @@ class LicensingServerSetup { | |||
|
|
|||
| let servicesConfig = fs.readFileSync(servicesConfigPathTemplate).toString(); | |||
| servicesConfig = servicesConfig.replace('%URL%', unityLicensingServer); | |||
| servicesConfig = servicesConfig.replace('%LICENSE_PRODUCT_IDS%', unityLicensingProductIds); | |||
There was a problem hiding this comment.
Add input validation and maintain backward compatibility.
The current implementation has potential security and compatibility concerns:
- Direct string replacement without sanitization
- No handling of empty values
- No validation of product ID format
Consider applying these changes:
+ // Validate and sanitize product IDs
+ const sanitizedProductIds = unityLicensingProductIds
+ ?.split(',')
+ .map(id => id.trim())
+ .filter(id => /^[\w-]+$/.test(id)) // Allow only alphanumeric chars and hyphens
+ .join(',');
+
+ // Only replace if valid product IDs are provided
+ if (sanitizedProductIds) {
+ servicesConfig = servicesConfig.replace('%LICENSE_PRODUCT_IDS%', sanitizedProductIds);
+ } else {
+ // Maintain backward compatibility by using empty string
+ servicesConfig = servicesConfig.replace('%LICENSE_PRODUCT_IDS%', '');
+ }
- servicesConfig = servicesConfig.replace('%LICENSE_PRODUCT_IDS%', unityLicensingProductIds);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| servicesConfig = servicesConfig.replace('%LICENSE_PRODUCT_IDS%', unityLicensingProductIds); | |
| // Validate and sanitize product IDs | |
| const sanitizedProductIds = unityLicensingProductIds | |
| ?.split(',') | |
| .map(id => id.trim()) | |
| .filter(id => /^[\w-]+$/.test(id)) // Allow only alphanumeric chars and hyphens | |
| .join(','); | |
| // Only replace if valid product IDs are provided | |
| if (sanitizedProductIds) { | |
| servicesConfig = servicesConfig.replace('%LICENSE_PRODUCT_IDS%', sanitizedProductIds); | |
| } else { | |
| // Maintain backward compatibility by using empty string | |
| servicesConfig = servicesConfig.replace('%LICENSE_PRODUCT_IDS%', ''); | |
| } |
| LicensingServerSetup.Setup( | ||
| parameters.unityLicensingServer, | ||
| parameters.actionFolder, | ||
| parameters.unityLicensingProductIds, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding type safety and validation for unityLicensingProductIds.
While the change correctly implements the new parameter, consider adding validation to ensure the product IDs are in the correct format before passing them to Setup.
Consider wrapping the Setup call with validation:
LicensingServerSetup.Setup(
parameters.unityLicensingServer,
parameters.actionFolder,
- parameters.unityLicensingProductIds,
+ validateProductIds(parameters.unityLicensingProductIds),
);
+ function validateProductIds(ids: string): string {
+ if (!ids) return '';
+ const productIds = ids.split(',').map(id => id.trim());
+ if (productIds.some(id => !/^[\w-]+$/.test(id))) {
+ throw new Error('Invalid product ID format. Product IDs should only contain alphanumeric characters, hyphens and underscores.');
+ }
+ return productIds.join(',');
+ }Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
I'm not sure about this suggestion as I don't have much details about unityLicensingProductIds's actual format, but it sounds a like a good idea to do input validation.
On the other hand, we're not doing validation for other parameters so this might be out of scope. Feel free to discard.
edit: I've deleted this learning as not relevant. Suggesting input validation is usually good ⬇
There was a problem hiding this comment.
@GabLeRoux, understood. Since input validation for other parameters is not currently implemented and might be out of scope for this PR, it's acceptable to discard this suggestion. Thank you for clarifying.
✏️ Learnings added
Learnt from: GabLeRoux
PR: game-ci/unity-test-runner#282
File: src/model/docker.ts:37-41
Timestamp: 2024-10-27T16:14:24.321Z
Learning: Input validation for parameters such as `unityLicensingProductIds` is not currently implemented in the codebase, so suggesting to add validation might be out of scope during code reviews.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Changes
This change enables supporting unity floating license servers with multiple products configured, more info here.
unityLicensingProductIdsservices-config.json.templateto set toolset based on unityLicensingProductIds. If this value is set to an empty string, it will use the server's default configuration, the same behavior as if the parameter was not included at alllRelated PRs
Related Issues
Successful Workflow Run Link
PRs don't have access to secrets so you will need to provide a link to a successful run
of the workflows from your own repo.
Checklist
in the documentation repo)
Summary by CodeRabbit
New Features
Bug Fixes
Documentation