Support multiple licensing products #661
Conversation
|
|
|
I don't have any knowledge about the floating license server or multiple configured products. Would it be possible to link relevant documentation or howtos from Unity? Could someone verify that this change doesn't affect other use cases? |
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the 📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a new optional input parameter, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Action
participant BuildParameters
participant ImageEnvironmentFactory
participant PlatformSetup
User->>Action: Trigger Action with unityLicensingProductIds
Action->>BuildParameters: Create instance with unityLicensingProductIds
BuildParameters-->>Action: Return instance with product IDs
Action->>ImageEnvironmentFactory: Get environment variables
ImageEnvironmentFactory-->>Action: Return environment variables including UNITY_LICENSING_PRODUCT_IDS
Action->>PlatformSetup: Setup platform with new variables
PlatformSetup-->>Action: Complete setup
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: 1
🧹 Outside diff range and nitpick comments (6)
src/model/platform-setup.ts (1)
Line range hint
1-42: Consider enhancing error handling and documentationWhile the overall implementation is solid, consider the following suggestions for improvement:
- Add a comment explaining the purpose of the '%LICENSE_PRODUCT_IDS%' replacement for better code readability.
- Consider adding error handling for the case where
buildParameters.unityLicensingProductIdsmight be undefined.- Add logging (using
core.debug) for the replacement operation to aid in troubleshooting.Here's a suggested implementation incorporating these improvements:
let servicesConfig = fs.readFileSync(servicesConfigPathTemplate).toString(); servicesConfig = servicesConfig.replace('%URL%', buildParameters.unityLicensingServer); -servicesConfig = servicesConfig.replace('%LICENSE_PRODUCT_IDS%', buildParameters.unityLicensingProductIds); +// Replace the license product IDs placeholder with the user-specified values or an empty string if not provided +const licenseProductIds = buildParameters.unityLicensingProductIds || ''; +servicesConfig = servicesConfig.replace('%LICENSE_PRODUCT_IDS%', licenseProductIds); +core.debug(`Replaced %LICENSE_PRODUCT_IDS% with: ${licenseProductIds}`); fs.writeFileSync(servicesConfigPath, servicesConfig);These changes will make the code more robust and easier to maintain in the future.
src/model/image-environment-factory.ts (1)
32-35: LGTM! Consider a minor improvement for consistency.The addition of the
UNITY_LICENSING_PRODUCT_IDSenvironment variable is well-implemented and aligns with the PR objectives to support multiple licensing products. The change is consistent with the existing pattern and doesn't affect other parts of the code.For consistency with other Unity-related environment variables, consider moving this new variable next to
UNITY_LICENSING_SERVER. This grouping could improve readability:{ name: 'UNITY_LICENSING_SERVER', value: parameters.unityLicensingServer, }, +{ + name: 'UNITY_LICENSING_PRODUCT_IDS', + value: parameters.unityLicensingProductIds, +}, { name: 'SKIP_ACTIVATION', value: parameters.skipActivation },src/model/input.ts (1)
126-128: LGTM! Consider adding a brief comment.The implementation of
unityLicensingProductIdsis correct and consistent with other methods in the class. It properly handles the case when the input is not provided.Consider adding a brief comment to explain the purpose of this new input, for example:
/** * Get the Unity licensing product IDs for floating license servers with multiple products. * @returns {string} Comma-separated list of product IDs or an empty string if not specified. */ static get unityLicensingProductIds(): string { return Input.getInput('unityLicensingProductIds') ?? ''; }action.yml (1)
254-260: LGTM! Consider a minor clarification in the description.The addition of the
unityLicensingProductIdsparameter is well-implemented and aligns with the PR objectives. It's correctly set as optional with a default empty string, which maintains backward compatibility.Consider slightly rewording the description for clarity:
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.' + 'Comma-separated list of license product identifiers to request from the license server. If not set, the default + license product configured on the licensing server will be requested. This parameter is only applicable when + unityLicensingServer is set.'This rewording maintains the same information while improving readability slightly.
src/model/build-parameters.ts (1)
25-25: Summary: Changes align well with PR objectives.The addition of the
unityLicensingProductIdsproperty to theBuildParametersclass successfully implements support for multiple licensing products as described in the PR objectives. The changes are minimal, focused, and consistent with the existing codebase structure.To further improve this PR:
- Consider adding a brief comment explaining the purpose of
unityLicensingProductIds.- Ensure that this new property is properly utilized in other relevant parts of the codebase (e.g., in the
services-config.json.templatefile mentioned in the PR summary).- Update any relevant documentation to reflect this new capability for Unity floating license servers with multiple products.
Also applies to: 152-152
src/model/build-parameters.test.ts (1)
200-200: Approved: Improved test isolation, but consider broader test setup improvements.The modification to delete the
UNITY_LICENSEenvironment variable before the test runs is a good practice for ensuring test isolation. However, the comment suggests that this variable is set for every test, which might indicate a need for better overall test isolation.Consider refactoring the test setup to avoid setting environment variables globally for all tests. Instead, you could use a
beforeEachhook to set up the necessary environment for each test, and aafterEachhook to clean up. This would improve test isolation and reduce the need for inline environment variable management. For example:describe('BuildParameters', () => { const originalEnv = process.env; beforeEach(() => { process.env = { ...originalEnv }; process.env.UNITY_LICENSE = testLicense; }); afterEach(() => { process.env = originalEnv; }); // ... existing tests ... it('throws error when no unity license provider provided', async () => { delete process.env.UNITY_LICENSE; await expect(BuildParameters.create()).rejects.toThrowError(); }); });This approach ensures each test starts with a clean environment and reduces the risk of tests interfering with each other.
📜 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 (7)
- action.yml (1 hunks)
- src/model/build-parameters.test.ts (1 hunks)
- src/model/build-parameters.ts (2 hunks)
- src/model/image-environment-factory.ts (1 hunks)
- src/model/input.ts (1 hunks)
- src/model/platform-setup.ts (1 hunks)
- src/model/platform-setup/setup-mac.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (10)
src/model/image-environment-factory.ts (1)
32-35: Verify integration and maintain backwards compatibilityWhile the implementation looks good, consider the following points to ensure smooth integration and backwards compatibility:
- Verify that the
unityLicensingProductIdsparameter is properly handled in other parts of the codebase whereDockerParametersis used.- Ensure that the default behavior (when
unityLicensingProductIdsis not provided) remains unchanged to maintain backwards compatibility.To help with verification, you can run the following script:
This script will help identify other places in the codebase where
unityLicensingProductIdsis used and whereDockerParametersis implemented, allowing you to verify proper integration and default behavior handling.✅ Verification successful
Verification Successful: No Issues Found
All usages of
unityLicensingProductIdsandDockerParametershave been verified across the codebase. The default behavior remains unchanged, ensuring backwards compatibility.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of unityLicensingProductIds and DockerParameters # Test 1: Check for other occurrences of unityLicensingProductIds echo "Checking for other occurrences of unityLicensingProductIds:" rg "unityLicensingProductIds" --type typescript # Test 2: Check for usage of DockerParameters echo "\nChecking for usage of DockerParameters:" rg "DockerParameters" --type typescript # Test 3: Check for potential default value handling echo "\nChecking for potential default value handling:" rg "unityLicensingProductIds.*=.*undefined" --type typescriptLength of output: 585
Script:
#!/bin/bash # Description: Check usage of unityLicensingProductIds and DockerParameters # Test 1: Check for other occurrences of unityLicensingProductIds echo "Checking for other occurrences of unityLicensingProductIds:" rg "unityLicensingProductIds" --type ts # Test 2: Check for usage of DockerParameters echo "\nChecking for usage of DockerParameters:" rg "DockerParameters" --type ts # Test 3: Check for potential default value handling echo "\nChecking for potential default value handling:" rg "unityLicensingProductIds\\s*=\\s*undefined" --type tsLength of output: 2165
src/model/platform-setup/setup-mac.ts (1)
171-171: LGTM with suggestions for improvementThe addition of the
UNITY_LICENSING_PRODUCT_IDSenvironment variable aligns well with the PR objective of supporting multiple licensing products. The implementation follows the existing pattern and naming conventions, which is good for consistency.However, I have a couple of suggestions to enhance this change:
Consider adding validation for the
unityLicensingProductIdsvalue before setting the environment variable. This could help prevent potential issues with invalid input.It would be beneficial to verify that this change doesn't negatively impact existing functionality, especially for users who don't specify licensing product IDs.
To ensure this change doesn't affect existing functionality, we can check for any conditional logic related to this new environment variable:
This will help us understand how this new environment variable is being used and if there are any potential issues with existing code.
✅ Verification successful
Change Verified Successfully
The addition of the
UNITY_LICENSING_PRODUCT_IDSenvironment variable has been verified. No conditional usages or impacts on existing functionality were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of UNITY_LICENSING_PRODUCT_IDS in the codebase rg "UNITY_LICENSING_PRODUCT_IDS" --type tsLength of output: 252
Script:
#!/bin/bash # Display the contents of src/model/image-environment-factory.ts for further analysis cat src/model/image-environment-factory.tsLength of output: 4934
Script:
#!/bin/bash # Search for conditional usages of UNITY_LICENSING_PRODUCT_IDS rg "UNITY_LICENSING_PRODUCT_IDS" --type ts | rg -e "if\s*\(" -e "\?\?" -A2 # Search for tests referencing UNITY_LICENSING_PRODUCT_IDS rg "UNITY_LICENSING_PRODUCT_IDS" --glob "*.{test,spec}.ts"Length of output: 133
Script:
#!/bin/bash # Search for any conditional usages of UNITY_LICENSING_PRODUCT_IDS in TypeScript files rg "UNITY_LICENSING_PRODUCT_IDS" --type ts | rg -e "if\s*\(" -e "switch\s*\(" -e "\?\?" -e "&&" -e "||" -A2 # Additionally, search for any function calls or manipulations involving UNITY_LICENSING_PRODUCT_IDS rg "UNITY_LICENSING_PRODUCT_IDS" --type ts | rg -e "\." -e "=" -e "\+" -e "-"Length of output: 601
src/model/input.ts (1)
126-128: Verify usage of the new input in related files.The addition of
unityLicensingProductIdsis correct and doesn't affect existing functionality. However, to ensure full implementation:Please run the following script to check for proper usage of the new input in other relevant files:
Ensure that:
unityLicensingProductIdsis properly defined as an input inaction.yml.- The new input is used in relevant TypeScript files where license-related logic is handled.
- Documentation is updated to mention this new capability.
action.yml (2)
Line range hint
261-264: LGTM! Indentation correction improves readability.The indentation correction for the
dockerWorkspacePathdescription improves the overall consistency and readability of the file.
254-260: Changes align with PR objectives and address raised concerns.The addition of the
unityLicensingProductIdsparameter fully aligns with the PR objectives to support multiple licensing products for Unity floating license servers. This implementation addresses the core functionality described in the PR summary.Regarding the concern raised by @webbertakken about the impact on other use cases:
- The parameter is optional, maintaining backward compatibility.
- When not set, it defaults to the server's default configuration, preserving existing behavior.
To further verify the impact on existing functionalities, we can run the following script to check for any unintended changes in other parts of the codebase:
This script will help ensure that the changes are isolated to the intended functionality and haven't inadvertently affected other parts of the codebase.
src/model/build-parameters.ts (3)
25-25: LGTM: New property declaration is consistent with existing code style.The
unityLicensingProductIdsproperty is correctly declared as public and uses the non-null assertion operator, which is consistent with other properties in the class. The string type is appropriate for storing product IDs.
152-152: LGTM: New property correctly integrated into thecreatemethod.The
unityLicensingProductIdsproperty is properly added to the return object in thecreatemethod. Its value is correctly sourced fromInput.unityLicensingProductIds, consistent with how other properties are handled.
25-25: Consider adding documentation for the new property.While the addition of
unityLicensingProductIdsis implemented correctly, it would be beneficial to add a comment explaining its purpose and expected usage. This would improve code maintainability and help other developers understand its role in the system.Additionally, to ensure this change is fully integrated:
Let's verify if this new property is used in other parts of the codebase:
Also applies to: 152-152
src/model/build-parameters.test.ts (2)
203-209: LGTM: New test case forunityLicensingProductIdsis well-implemented.The new test case correctly verifies that the
BuildParameters.create()method returns theunityLicensingProductIdsas expected. It follows the established pattern of other tests in this file, mocking the input and asserting the output appropriately.
Line range hint
1-238: Summary: Good additions and improvements, with a suggestion for further enhancement.The changes in this file appropriately support the PR objective of handling multiple licensing products. The new test case for
unityLicensingProductIdsis well-implemented and increases test coverage. The modification to the existing unity license provider test improves isolation.However, there's an opportunity to enhance the overall test setup to ensure better isolation between all tests. Consider implementing a more robust setup/teardown mechanism as suggested in the previous comment.
Overall, these changes are approved with the recommendation to consider the suggested improvement for test isolation.
🧰 Tools
🪛 Biome
[error] 213-213: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
Yeah so here's Unity's docs on floating licensing servers: https://docs.unity.com/licensing/en-us/manual I had linked this page which explains how to a floating licensing server can be configured to have multiple 'products' AKA 'toolsets' (I like to think of these as license pools). This is also a good one to explain the configuration options for the client I think the relevant things to know here are that you can self-host a floating licensing server and that server can house multiple products which each have certain number of licenses/seats. Clients can lease a license from the server as long as they have the
This is just an anecdote, but I've tested that having the |
|
@coderabbitai draw a flow chart for these changes |
|
Tip For best results, initiate chat on the files or code changes. @GabLeRoux, here's a flowchart illustrating the changes introduced in this PR: flowchart TD
subgraph Action Input
A[Add 'unityLicensingProductIds' to action.yml]
A --> B[Create Input getter 'unityLicensingProductIds' in input.ts]
end
B --> C[Update BuildParameters with 'unityLicensingProductIds' property]
C --> D[Modify ImageEnvironmentFactory to include 'UNITY_LICENSING_PRODUCT_IDS' env variable]
D --> E[Update PlatformSetup to replace '%LICENSE_PRODUCT_IDS%' in services-config.json]
E --> F[Update SetupMac to set 'UNITY_LICENSING_PRODUCT_IDS' environment variable]
F --> G[Unity Builder now supports multiple licensing products]
Let me know if you have any questions! |
# Conflicts: # dist/index.js.map
|
I've fixed the merge conflicts by simply merging |
Changes
This change enables supporting unity floating license servers with multiple products configured, more info here.
Related 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
code of conduct
in the documentation repo)
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests