Refactor orb to support proper activation of unity using serial #83
Conversation
WalkthroughThe changes primarily involve updates to the CircleCI configuration and related scripts for managing Unity licenses. The Changes
Possibly related issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 9
🧹 Outside diff range and nitpick comments (16)
src/scripts/linux/return-license.sh (3)
Line range hint
5-15: Security: Prevent credential exposure in logsThe
set -xdebugging output will expose sensitive credentials (password and serial) in the logs.Consider this safer approach:
-set -x +# Temporarily disable command traces to avoid exposing credentials +set +x unity-editor \ -quit \ -batchmode \ -nographics \ -returnlicense \ -username "$unity_username" \ -password "$unity_password" \ -serial "$unity_serial" \ -logfile /dev/stdout +# Re-enable command traces +set -x
Line range hint
1-16: Add documentation for Unity activation parametersThe script would benefit from comments explaining the purpose of each parameter and why both password and serial are needed for license return.
Consider adding a header comment block:
#!/bin/false # shellcheck shell=bash # shellcheck disable=SC2154 + +# Script: return-license.sh +# Purpose: Returns Unity license using both user credentials and serial number +# Parameters: +# unity_username: Unity account username +# unity_password: Unity account password +# unity_serial: Unity serial number for activation
Using
-passwordtwice for both password and serial is incorrectBased on the analysis of the prepare-env.sh scripts across different platforms, the correct pattern for Unity license activation and return is:
- For activation:
-username,-password, and-serialparameters are used- For return:
-username,-password, and-returnlicenseparameters should be usedThe current implementation in both Linux and macOS return-license.sh scripts incorrectly uses
-passwordtwice, where the second occurrence should be removed as the serial is not needed for license return.Locations needing fixes:
src/scripts/linux/return-license.sh: Remove the line-password "$unity_serial" \src/scripts/macos/return-license.sh: Remove the line-password "$unity_serial" \🔗 Analysis chain
Line range hint
1-16: Verify the license return process with Unity supportThe combination of both password and serial for license return seems unusual. We should verify this approach with Unity's documentation or support.
Let's search for related documentation or discussions:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Unity license return documentation in the repository rg -i "unity.*license.*return|return.*unity.*license" --type md # Search for any existing Unity serial activation examples rg -i "unity.*serial.*activation|activation.*unity.*serial" --type md # Check if there are any other scripts using similar activation patterns fd -e sh -e bash | xargs rg -l "unity.*serial"Length of output: 386
Script:
#!/bin/bash # Let's check the content of prepare-env.sh files to understand the activation pattern echo "=== Linux prepare-env.sh ===" cat src/scripts/linux/prepare-env.sh echo -e "\n=== Windows prepare-env.sh ===" cat src/scripts/windows/prepare-env.sh echo -e "\n=== macOS prepare-env.sh ===" cat src/scripts/macos/prepare-env.sh # Also check other return-license scripts echo -e "\n=== macOS return-license.sh ===" cat src/scripts/macos/return-license.shLength of output: 13886
src/scripts/macos/return-license.sh (2)
Line range hint
1-16: Security: Debug output exposes sensitive informationThe
set -xdebug flag will print all commands and their arguments to stdout, potentially exposing sensitive information (password and serial) in the CI logs.Consider either:
- Removing debug output entirely, or
- Temporarily disabling debug mode for sensitive commands:
#!/bin/false # shellcheck shell=bash # shellcheck disable=SC2154 -set -x +# Temporarily disable debug output for sensitive commands +set +x # Return license "$UNITY_EDITOR_PATH" \ -quit \ -batchmode \ -nographics \ -returnlicense \ -username "$unity_username" \ -password "$unity_password" \ -serial "$unity_serial" \ -logfile /dev/stdout -set +x +# Re-enable debug output if needed +set -x
Line range hint
1-16: Add error handling for missing credentialsThe script should validate that required credentials are provided before attempting to return the license.
Consider adding validation:
#!/bin/false # shellcheck shell=bash # shellcheck disable=SC2154 +# Validate required credentials +if [ -z "$unity_username" ] || [ -z "$unity_password" ] || [ -z "$unity_serial" ]; then + echo "Error: Missing required credentials. Please ensure unity_username, unity_password, and unity_serial are set." + exit 1 +fi + set +x # Return license "$UNITY_EDITOR_PATH" \src/commands/return-license.yml (1)
21-22: Move TODO to issue tracker for better trackingThe TODO comment contains valuable implementation details about license serial extraction. This should be tracked properly in the issue tracker rather than in the code.
Would you like me to create a GitHub issue to track this improvement?
src/examples/test_and_build_personal_license.yml (3)
Line range hint
54-74: Consider using consistent Windows executor versionsWhile the
unity-serial-var-nameparameter is correctly added, there's an inconsistency in the Windows executor versions:
- Test job uses
windows-2022- Build job uses
windows-2019This might lead to environment inconsistencies between testing and building phases.
Consider using the same Windows version for both jobs to ensure consistent behavior. Preferably use the newer
windows-2022for both jobs.
86-86: Update documentation to reflect the new serial parameterThe
unity-serial-var-nameparameter is correctly added to the macOS jobs. However, since this is an example file, it would be helpful to update the documentation link in the description to include information about the new serial parameter setup.Consider adding a note in the description about the required
UNITY_SERIALenvironment variable:description: > Drop-in solution to automatically run tests and build your Unity project using a Personal license. Make sure to read the documentation before using the orb: https://game.ci/docs/circleci/getting-started. + Required environment variables: UNITY_USERNAME, UNITY_PASSWORD, and UNITY_SERIAL. For runner examples, see: https://game.ci/docs/circleci/executors#macos-runner and https://game.ci/docs/circleci/executors#windows-runner.Also applies to: 99-99
Line range hint
1-108: Consider parameterizing the Unity versionThe Unity version
2022.3.33f1is hardcoded across all jobs. To improve maintainability, consider extracting it into a parameter or environment variable.This would make it easier to update the Unity version across all jobs and reduce the risk of version mismatches. Example:
parameters: unity_version: type: string default: "2022.3.33f1"Then use
<< parameters.unity_version >>in place of the hardcoded version.src/jobs/build.yml (2)
29-32: Improve documentation format and clarityWhile the added documentation is helpful, consider restructuring it for better clarity:
- Move the TODO and technical details to a separate documentation file
- Keep the parameter description focused on what users need to know
- Add a link to comprehensive documentation about license setup
Consider this format:
- Required even for personal license. - TODO: Improve documentation on how to extract from personal license, it's something like this: - cat Unity_lic.ulf | grep DeveloperData | sed -E 's/.*Value="([^"]+)".*/\1/' | base64 --decode - For future versions, we could simply request the user to provide the ulf file and we could do the extraction. + Required even for personal licenses. For detailed instructions on obtaining your + Unity serial number, please refer to our documentation at: + https://game.ci/docs/circleci/activation
Inconsistent license-to-serial transition detected
The codebase still contains references to the old license-based activation that need to be addressed:
src/commands/prepare-env.ymlstill defines and uses the removed parameterunity-license-var-namein its environment variablessrc/scripts/linux/prepare-env.shcontains mixed logic handling both license files and serial activation- Several files still contain
.ulfreferences indicating incomplete transition🔗 Analysis chain
Line range hint
1-150: Verify complete transition from license to serial activationThe removal of the
unity-license-var-nameparameter represents a significant change in the activation process. Let's verify that all related components are updated consistently.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify complete removal of license-based activation references # Check for any remaining references to the old license-based activation echo "Checking for remaining references to unity-license-var-name:" rg "unity-license-var-name" -l # Check for any remaining references to license files echo "Checking for remaining references to license files:" rg -i "\.ulf" -l # Check for consistency in serial parameter naming echo "Checking for consistency in serial parameter naming:" rg "unity-serial" -lLength of output: 759
Script:
#!/bin/bash # Let's check the content of prepare-env.yml to see if it still uses unity-license-var-name echo "Content of prepare-env.yml:" cat src/commands/prepare-env.yml # Let's also check the content of prepare-env.sh to understand the license file references echo -e "\nContent of prepare-env.sh:" cat src/scripts/linux/prepare-env.shLength of output: 6300
src/scripts/macos/prepare-env.sh (2)
Line range hint
82-96: Clean up unused variable assignmentThe
resolved_unity_serialis still being assigned but no longer used since the activation command now usesunity_serialdirectly. This could lead to confusion.Consider applying this diff:
if [ -n "$unity_serial" ]; then printf '%s\n' "Detected Unity serial." - readonly resolved_unity_serial="$unity_serial" + # Serial will be used directly in activation # License provided. elif [ -n "$unity_encoded_license" ]; then printf '%s\n' "No serial detected. Extracting it from the encoded license." if ! extract_serial_from_license; then printf '%s\n' "Failed to parse the serial from the Unity license." printf '%s\n' "Please try again or open an issue." printf '%s\n' "See the docs for more details: https://game.ci/docs/circleci/activation#personal-license" return 1
Line range hint
1-165: Document the activation strategy changeThe shift from license-based to serial-based activation represents a significant architectural change. Consider adding comments or updating documentation to:
- Explain why serial-based activation is preferred
- Document the expected format and source of the serial
- Update any related documentation about CircleCI secret configuration
Would you like me to help draft the documentation updates?
.circleci/test-deploy.yml (2)
Line range hint
337-348: Review tvOS build configuration comment.The tvOS build job is commented out with a Windows-related TODO comment, but it's using a Windows executor which seems incorrect for tvOS builds.
Consider this alternative configuration:
- unity/build: name: "build-tvOS" step-name: "Build Apple's tvOS" unity-username-var-name: "UNITY_USERNAME" unity-password-var-name: "UNITY_PASSWORD" unity-serial-var-name: "UNITY_SERIAL" executor: - name: "unity/windows-2019" - size: "large" + name: "unity/macos" + resource_class: "macos.m1.medium.gen1" editor_version: "2022.3.33f1" target_platform: "appletv"
Windows test coverage gap confirmed with known activation issues
The codebase shows Windows-related test configurations are intentionally commented out with explicit TODO comments about investigating Windows activation errors. This is a known issue affecting both test and build jobs:
- Windows test jobs are commented out with
# TODO: Investigate Windows activation error- Windows build jobs (IL2CPP, tvOS, custom method) are similarly disabled
- The issue is tracked in the release requirements section
The gap in Windows testing coverage is a legitimate concern. Consider:
- Opening a GitHub issue to track the Windows activation investigation if not already done
- Adding temporary cross-platform testing using the Linux executor with
windows-monotarget platform (as seen in thebuild-Windows64-monojob) until the Windows executor issues are resolved- Documenting this known limitation in the README to set proper expectations for users
🔗 Analysis chain
Line range hint
114-129: Address Windows test coverage gap.The Windows test jobs have been commented out with a TODO about activation errors. This creates a gap in the testing coverage for Windows platforms.
Let's check if there are any related issues or discussions about Windows activation:
Consider:
- Adding temporary alternative Windows testing strategy
- Creating a tracking issue for the Windows activation problem
- Documenting the known limitations in the README
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for related issues about Windows activation gh issue list --repo game-ci/unity-orb --search "windows activation in:title,body" # Search for discussions about Windows activation in the codebase rg -i "windows.*activation|activation.*windows" --type yamlLength of output: 146
Script:
#!/bin/bash # Let's check for any Windows-related test configurations and discussions rg -i "windows.*test|test.*windows" --type yaml -A 5 -B 5 # Search for any TODO or related comments about Windows testing rg -i "todo.*windows|windows.*todo" --type yaml # Check if there are other test configurations in the file cat .circleci/test-deploy.ymlLength of output: 23836
src/scripts/linux/prepare-env.sh (1)
27-31: Ensure robust extraction of serial number from licenseThe command used to extract the serial number from the license file may fail if the license format changes or if
SerialNumberis not present. This could result in activation failures.Consider adding error handling or making the extraction more robust.
Possible fix:
unity_serial=$(echo "$unity_license" | grep -oP '(?<=SerialNumber=)[^\r\n]*') if [ -z "$unity_serial" ]; then echo "Failed to extract serial number from the encoded license file." return 1 fiAdditionally, you could validate that
unity_serialmatches the expected format.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
.circleci/test-deploy.yml(22 hunks)src/commands/prepare-env.yml(0 hunks)src/commands/return-license.yml(1 hunks)src/examples/test_and_build_personal_license.yml(6 hunks)src/examples/test_and_build_pro_or_plus_license.yml(4 hunks)src/jobs/build.yml(1 hunks)src/jobs/create-activation-file.yml(0 hunks)src/jobs/test.yml(1 hunks)src/scripts/linux/prepare-env.sh(1 hunks)src/scripts/linux/return-license.sh(1 hunks)src/scripts/macos/prepare-env.sh(1 hunks)src/scripts/macos/return-license.sh(1 hunks)
💤 Files with no reviewable changes (2)
- src/commands/prepare-env.yml
- src/jobs/create-activation-file.yml
🔇 Additional comments (11)
src/scripts/macos/return-license.sh (1)
Line range hint 1-16: Verify Unity CLI parameters in the codebase
Let's verify how these parameters are used across the codebase to ensure consistency.
src/jobs/test.yml (2)
Line range hint 33-94: Implementation looks solid
The job structure, parameter definitions, and step sequence are well-organized and properly implemented. The conditional license return is a good practice to ensure proper cleanup.
Line range hint 1-32: Document breaking changes and provide migration guide
The removal of unity-license-var-name parameter and shift to serial-based activation is a breaking change that needs to be clearly documented.
Consider adding the following to the job description:
description: >
Simple drop-in job to build your Unity game.
+
+ BREAKING CHANGE: This version requires a Unity serial number for activation.
+ Previous versions supporting direct license file usage have been deprecated.
+ See migration guide at: [URL to migration guide]Let's verify the impact of this change:
src/examples/test_and_build_pro_or_plus_license.yml (3)
54-54: LGTM: Unity serial configuration added for Windows jobs
The addition of unity-serial-var-name: "UNITY_SERIAL" to Windows test and build jobs aligns with the PR's objective to support proper Unity activation using serial numbers.
Also applies to: 68-68
86-86: LGTM: Unity serial configuration added for macOS jobs
The addition of unity-serial-var-name: "UNITY_SERIAL" to macOS test and build jobs maintains consistency with other platforms.
Also applies to: 99-99
57-57: Consider using consistent Windows executor versions
The Windows test job uses unity/windows-2022 while the build job uses unity/windows-2019. This inconsistency might lead to potential compatibility issues.
Let's verify if there are any known issues or recommendations regarding the executor versions:
Consider using the same Windows executor version for both jobs unless there's a specific reason for the difference. The newer unity/windows-2022 would be preferred for both jobs if there are no compatibility constraints.
Also applies to: 71-71
src/examples/test_and_build_personal_license.yml (1)
22-22: LGTM! Verify the UNITY_SERIAL secret in CircleCI
The addition of unity-serial-var-name parameter is consistent with the PR objectives. The parameter is correctly configured for both test and build jobs.
Run this script to verify the secret is properly configured:
Also applies to: 36-36
src/scripts/macos/prepare-env.sh (1)
165-165: Verify error handling for missing serial
The change from resolved_unity_serial to unity_serial aligns with the PR objectives. However, we should verify the error handling when the serial is missing.
✅ Verification successful
Let me gather more information to verify the error handling for missing serial.
Error handling for missing serial is properly implemented
The code has comprehensive error handling for missing serial through multiple validation layers:
- Initial validation in
resolve_unity_serial()checks for both serial and encoded license - Proper error messages and exit codes when neither serial nor license is provided
- Graceful handling when serial extraction from license fails
- Clear user-facing error messages with documentation links
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all error handling cases related to unity_serial
rg -A 5 "unity_serial" | grep -B 2 -A 2 "error\|fail\|exit"
# Search for any validation of unity_serial before usage
ast-grep --pattern 'if [ -n "$unity_serial" ]'
Length of output: 996
Script:
#!/bin/bash
# Search for unity_serial validation and usage context
rg -B 5 -A 5 "unity_serial"
# Search for any other error handling patterns related to Unity activation
rg -B 3 -A 3 "Unity.*activation|activate.*Unity"
Length of output: 13561
.circleci/test-deploy.yml (3)
Line range hint 202-214: Consider impact of disabled Windows IL2CPP builds.
The Windows IL2CPP build job has been commented out. This could impact users who need to build Windows games with IL2CPP backend.
Let's check if there are any dependencies on Windows IL2CPP builds:
Consider:
- Adding a note in the documentation about this limitation
- Creating a tracking issue for restoring Windows IL2CPP support
- Providing alternative build strategies for Windows IL2CPP users
119-120:
Investigate duplicate serial configuration.
There appears to be both unity-serial-var-name and serial-password-var-name configured for Windows tests. This looks like a potential oversight that could cause confusion.
Remove the duplicate line:
# unity-serial-var-name: "UNITY_SERIAL"
- # serial-password-var-name: "UNITY_SERIAL"Likely invalid or redundant comment.
Line range hint 474-492: Systematically address Windows activation issues.
Multiple Windows-related jobs are commented out with TODOs about activation errors. This suggests a systematic issue with Windows activation that needs to be addressed.
Let's analyze the Windows activation setup:
Recommendations:
- Create a comprehensive tracking issue for Windows activation
- Document the current limitations and workarounds
- Consider adding a Windows-specific activation guide
- Add tests to verify activation works before enabling Windows jobs
| unity-serial-var-name: | ||
| type: env_var_name | ||
| default: "UNITY_SERIAL" | ||
| description: | | ||
| Enter the name of the environment variable containing your Unity serial number. | ||
| Required even for personal license. | ||
| TODO: Improve documentation on how to extract from personal license, it's something like this: | ||
| cat Unity_lic.ulf | grep DeveloperData | sed -E 's/.*Value="([^"]+)".*/\1/' | base64 --decode | ||
| For future versions, we could simply request the user to provide the ulf file and we could do the extraction. |
There was a problem hiding this comment.
💡 Codebase verification
Parameter unity-serial-var-name is defined but not properly integrated
The parameter is defined in the YAML but not passed to the environment variables in the steps section. Additionally, the shell scripts are using a hardcoded unity_serial variable instead of expanding the environment variable name parameter. This needs to be fixed:
- In
src/commands/return-license.yml: AddPARAM_UNITY_SERIAL_VAR_NAME: << parameters.unity-serial-var-name >>to the environment variables - In
src/scripts/return-license.sh: Addreadonly unity_serial="${!PARAM_UNITY_SERIAL_VAR_NAME}"along with other variable expansions - In platform-specific scripts (
linux/return-license.sh,macos/return-license.sh): Update to use the expanded$unity_serialvariable consistently - In
windows/return-license.sh: Add-serial $Env:UNITY_SERIALto the Unity command parameters
🔗 Analysis chain
Verify integration of unity-serial-var-name parameter
The new parameter is defined but not explicitly passed to the environment variables in the steps section. Let's verify its usage in the shell scripts.
Also applies to: 24-35
Let me try a different approach to locate the relevant files first.
Now let's check the actual usage of the parameter in the relevant shell scripts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if unity-serial-var-name is properly used in shell scripts
echo "Checking shell scripts for UNITY_SERIAL usage..."
# Check all shell scripts referenced in the YAML
for script in "scripts/linux/return-license.sh" "scripts/windows/return-license.sh" "scripts/macos/return-license.sh" "scripts/return-license.sh" "scripts/utils.sh"; do
echo "\nChecking $script:"
if [ -f "$script" ]; then
# Look for UNITY_SERIAL or unity_serial usage
rg -i "unity.?serial" "$script" || echo "No direct references found"
# Look for parameter usage
rg "PARAM_UNITY_SERIAL_VAR_NAME" "$script" || echo "No parameter references found"
else
echo "Script not found: $script"
fi
done
Length of output: 2101
Script:
#!/bin/bash
# Find all shell scripts in the repository
echo "Finding all shell scripts in the repository..."
fd -e sh
# Find all YAML files that might reference the parameter
echo -e "\nFinding YAML files that mention unity-serial..."
rg -l -i "unity.?serial" -g "*.yml" -g "*.yaml"
Length of output: 1010
Script:
#!/bin/bash
# Check the content of return-license scripts
echo "Checking src/scripts/return-license.sh..."
cat src/scripts/return-license.sh
echo -e "\nChecking src/scripts/linux/return-license.sh..."
cat src/scripts/linux/return-license.sh
echo -e "\nChecking src/scripts/macos/return-license.sh..."
cat src/scripts/macos/return-license.sh
echo -e "\nChecking src/scripts/windows/return-license.sh..."
cat src/scripts/windows/return-license.sh
# Also check the YAML file to see how the parameter is passed
echo -e "\nChecking how the parameter is used in return-license.yml..."
cat src/commands/return-license.yml
Length of output: 4212
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/scripts/macos/prepare-env.sh (1)
82-84: Consider maintaining immutability with readonly.The logic for detecting and assigning the Unity serial is correct. However, consider maintaining immutability by keeping the
readonlymodifier forresolved_unity_serialconsistent across all code paths.- resolved_unity_serial="$unity_serial" + readonly resolved_unity_serial="$unity_serial"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
src/commands/prepare-env.yml(0 hunks)src/scripts/linux/return-license.sh(1 hunks)src/scripts/macos/prepare-env.sh(1 hunks)src/scripts/macos/return-license.sh(1 hunks)src/scripts/prepare-env.sh(0 hunks)src/scripts/return-license.sh(1 hunks)
💤 Files with no reviewable changes (2)
- src/commands/prepare-env.yml
- src/scripts/prepare-env.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- src/scripts/linux/return-license.sh
- src/scripts/macos/return-license.sh
🔇 Additional comments (5)
src/scripts/return-license.sh (2)
15-15: LGTM! Verify platform-specific implementations.
The new unity_serial variable is correctly declared as readonly and follows the established pattern for environment variable expansion.
Let's verify the implementation in platform-specific scripts:
✅ Verification successful
Unity serial parameter correctly implemented across platform-specific scripts
The unity_serial variable is properly utilized in both Linux and macOS implementations as a parameter to the license return command. The Windows implementation doesn't show usage, which is expected as the PR focuses on serial-based activation.
src/scripts/linux/return-license.sh: Uses-serial "$unity_serial"src/scripts/macos/return-license.sh: Uses-serial "$unity_serial"src/scripts/windows/return-license.sh: No serial parameter (as expected)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how the unity_serial variable is used in platform-specific scripts
# Check for the presence of serial parameter in platform-specific scripts
echo "Checking Linux implementation:"
rg -A 5 "unity_serial" "$SCRIPT_RETURN_LICENSE_LINUX"
echo "Checking MacOS implementation:"
rg -A 5 "unity_serial" "$SCRIPT_RETURN_LICENSE_MACOS"
echo "Checking Windows implementation:"
rg -A 5 "unity_serial" "$SCRIPT_RETURN_LICENSE_WINDOWS"
Length of output: 417
Script:
#!/bin/bash
# Let's first find the platform-specific scripts
echo "Finding platform-specific license return scripts:"
fd -t f "return.*license.*" --full-path
# Then examine their content
echo -e "\nChecking for unity_serial usage in these files:"
rg -l "unity_serial" $(fd -t f "return.*license.*" --full-path)
# Get the content of these files to verify implementation
echo -e "\nContent of files containing unity_serial:"
rg -A 5 "unity_serial" $(fd -t f "return.*license.*" --full-path)
Length of output: 1910
18-20: Ensure consistent newline handling across platforms.
The added newline after the conditional block is good for readability. However, ensure that line endings are consistently handled across different platforms (Linux, macOS, Windows) to prevent any script execution issues.
Let's check the line endings in the script:
✅ Verification successful
Line endings are consistently handled with Unix-style (LF) endings
The script uses consistent Unix-style line endings (LF) throughout the file, as shown by the $ markers in the cat -A output. There are no CRLF (^M$) line endings present, and the script maintains consistent line ending style across all platform-specific conditions. This is actually the preferred approach for shell scripts, even when running on Windows, as modern Windows environments (Git Bash, WSL) handle Unix-style endings correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check line endings in the script
# Check file format
file src/scripts/return-license.sh
# Check for mixed line endings
rg -l $'\r\n' src/scripts/return-license.sh
Length of output: 366
Script:
#!/bin/bash
# Let's try a different approach to check line endings
# Use od command to display file contents in octal format to check line endings
od -c src/scripts/return-license.sh | grep -E '\r|\n'
# Additionally, let's check if there are any CRLF line endings using grep
grep -U -l $'\r' src/scripts/return-license.sh || echo "No CRLF endings found"
Length of output: 250
Script:
#!/bin/bash
# Let's try one final approach using cat with special options to reveal line endings
# Show line endings using cat with -A option
cat -A src/scripts/return-license.sh
# Also check the file content to understand the script structure
cat src/scripts/return-license.sh
Length of output: 1354
src/scripts/macos/prepare-env.sh (3)
86-98: LGTM! Improved error handling and documentation.
The enhanced error handling with specific messages and documentation links will help users troubleshoot activation issues more effectively. The code structure properly propagates errors and maintains immutability in the successful path.
100-105: Verify the documentation links.
The error messages are clear and helpful. Let's verify that the documentation links are valid and point to the correct activation guides.
✅ Verification successful
Documentation links are valid and accessible
Both documentation URLs are accessible and return valid responses, confirming that users will be able to access the activation guides when following the error message instructions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the documentation links are valid
# Test: Check if the documentation URLs return valid responses
urls=(
"https://game.ci/docs/circleci/activation"
"https://game.ci/docs/circleci/activation#personal-license"
)
for url in "${urls[@]}"; do
if curl --output /dev/null --silent --head --fail "$url"; then
echo "✓ URL is valid: $url"
else
echo "✗ URL is not accessible: $url"
fi
done
Length of output: 640
82-105: Verify the Unity activation flow with both serial and license approaches.
The changes successfully support both direct serial activation and encoded license extraction, aligning with the PR's objective of proper Unity activation. Let's verify that both activation paths are working as expected in the CI environment.
✅ Verification successful
Unity activation implementation is correctly handling both serial and license scenarios
Based on the code analysis, the implementation properly supports both activation methods:
- Direct serial activation when
unity_username,unity_password, andunity_serialare provided - License-based activation with serial extraction when
unity_encoded_licenseis provided
The implementation is consistent across all platforms (Windows, macOS, Linux) with appropriate error handling and user guidance. The changes maintain backward compatibility while supporting the transition to serial-based activation.
Key points verified:
- Robust error handling with descriptive messages
- Proper serial extraction from encoded licenses
- Platform-specific implementations following the same pattern
- Clear documentation links for troubleshooting
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Unity activation flow in CI logs
# Test: Search for successful activation patterns in CI logs
# Check for successful activation patterns
rg -A 5 "Unity license" .circleci/
rg -A 5 "successfully activated" .circleci/
# Check for any activation-related errors or warnings
rg -i "failed.*activate" .circleci/
rg -i "error.*license" .circleci/
Length of output: 156
Script:
#!/bin/bash
# Let's analyze the activation script implementation and related files
# Check the full content of the activation script
cat src/scripts/macos/prepare-env.sh
# Look for any test files related to activation
fd -t f test activation
# Search for any activation-related functions or patterns
rg -A 5 "extract_serial_from_license|unity_serial|unity_encoded_license" src/
# Check for any documentation about activation
fd -t f README.md CONTRIBUTING.md docs
Length of output: 16695
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
src/jobs/test.yml (1)
Line range hint
1-86: Consider providing migration guidanceGiven the breaking changes in parameter naming and the shift to using serial numbers for activation, consider:
- Adding a migration guide in the README
- Providing examples of updated CircleCI configurations
- Documenting the rationale behind using serial numbers instead of license files
This will help users smoothly transition to the new version.
src/scripts/linux/prepare-env.sh (1)
98-102: Use versioned tags instead of commit hashesUsing a commit hash (
7fc1c95) for referencing external dependencies is not recommended as it:
- Makes it harder to track which version is being used
- Complicates the upgrade process
- Reduces transparency about compatibility
Consider using a tagged version instead:
# Define variables repo_url="https://gitlab.com/game-ci/unity3d-gitlab-ci-example" -ref="7fc1c95" # Will update to a tag once the latest version is merged +ref="v1.0.0" # Use appropriate version tag file_path="ci/before_script.sh" before_script="$base_dir/before_script.sh"src/examples/test_and_build_pro_or_plus_license.yml (1)
20-22: Maintain consistent quote style for variable namesThe changes look good and align with the new Unity activation approach. However, there's an inconsistency in quote styles:
unity-email-var-name: 'UNITY_EMAIL'uses single quotesunity-serial-var-name: "UNITY_SERIAL"uses double quotesConsider using single quotes consistently:
- unity-serial-var-name: "UNITY_SERIAL" + unity-serial-var-name: 'UNITY_SERIAL'Also applies to: 34-36
src/jobs/build.yml (1)
14-18: Update parameter description to match the new parameter nameThe parameter has been renamed from
unity-username-var-nametounity-email-var-name, but the description still refers to "Unity username". Update for consistency:description: | - Enter the name of the environment variable containing your Unity username. + Enter the name of the environment variable containing your Unity email..circleci/test-deploy.yml (1)
Line range hint
116-136: Consider tracking Windows activation issuesSeveral Windows-related jobs have been commented out with TODO comments indicating activation errors. While this is a valid temporary solution, these issues should be tracked for future resolution.
Would you like me to create GitHub issues to track the following Windows-related tasks?
- Investigate Windows activation error in test jobs
- Investigate Windows activation error in build jobs
- Investigate Windows activation error in tvOS build job
Also applies to: 200-214, 335-349, 371-385
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
.circleci/test-deploy.yml(22 hunks)src/commands/prepare-env.yml(2 hunks)src/commands/return-license.yml(1 hunks)src/examples/test_and_build_personal_license.yml(6 hunks)src/examples/test_and_build_pro_or_plus_license.yml(6 hunks)src/jobs/build.yml(4 hunks)src/jobs/test.yml(4 hunks)src/scripts/linux/prepare-env.sh(1 hunks)src/scripts/linux/return-license.sh(1 hunks)src/scripts/macos/prepare-env.sh(2 hunks)src/scripts/macos/return-license.sh(1 hunks)src/scripts/prepare-env.sh(1 hunks)src/scripts/return-license.sh(1 hunks)src/scripts/windows/prepare-env.sh(3 hunks)src/scripts/windows/return-license.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- src/commands/prepare-env.yml
- src/commands/return-license.yml
- src/examples/test_and_build_personal_license.yml
- src/scripts/linux/return-license.sh
- src/scripts/macos/prepare-env.sh
- src/scripts/macos/return-license.sh
- src/scripts/prepare-env.sh
- src/scripts/return-license.sh
🔇 Additional comments (16)
src/scripts/windows/return-license.sh (1)
25-25: LGTM: Container cleanup is appropriate
The container cleanup is implemented correctly using docker rm -f.
src/jobs/test.yml (3)
29-32: Reiterating previous documentation feedback
The previous review comment about improving the serial number extraction documentation is still valid and should be addressed.
70-72: LGTM! Verify command parameter definitions
The job step configurations correctly use the renamed parameters. The changes are consistent throughout the file.
Let's verify the command definitions match these parameters:
Also applies to: 85-86
✅ Verification successful
Parameter definitions match job configuration
The command definitions in prepare-env.yml and return-license.yml correctly match the parameter names used in the job configuration:
- Both commands define
unity-email-var-name,unity-password-var-name, andunity-serial-var-nameparameters - The parameters are properly passed to the environment variables in the command steps
- Default values are consistently set to "UNITY_EMAIL", "UNITY_PASSWORD", and "UNITY_SERIAL"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check prepare-env and return-license command definitions
echo "Checking prepare-env command definition:"
cat src/commands/prepare-env.yml
echo "Checking return-license command definition:"
cat src/commands/return-license.yml
Length of output: 4391
14-16: Breaking Change: Parameter renamed from username to email
The parameter rename from unity-username-var-name to unity-email-var-name better reflects Unity's authentication mechanism. However, this is a breaking change that requires updates in existing CircleCI workflows.
Let's check for potential impacts:
src/examples/test_and_build_pro_or_plus_license.yml (3)
84-86: LGTM! Consistent configuration for macOS jobs
The macOS configuration is well-structured with:
- Consistent use of variables across test and build jobs
- Appropriate resource class selection (macos.m1.medium.gen1)
- Proper alignment with the new Unity activation approach
Also applies to: 97-99
20-22: Verify CircleCI context variables
The configuration uses the following variables from the 'unity' context:
- UNITY_EMAIL (replacing UNITY_USERNAME)
- UNITY_PASSWORD
- UNITY_SERIAL
Let's verify these variables are properly set in the CircleCI context:
Also applies to: 34-36, 52-54, 66-68, 84-86, 97-99
✅ Verification successful
Migration from UNITY_USERNAME to UNITY_EMAIL is complete
The verification shows:
- No references to the old
UNITY_USERNAMEvariable exist in any YAML files - The
unitycontext is consistently used across all jobs in both personal and pro/plus license workflows - All build jobs correctly reference the new variables:
- UNITY_EMAIL
- UNITY_PASSWORD
- UNITY_SERIAL
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any references to old variable names to ensure complete migration
rg -i "unity[_-]username" --type yaml
# Search for context usage to verify consistency
rg "context: unity" --type yaml -A 5
Length of output: 5636
56-57: Verify Windows executor version differences
The test job uses unity/windows-2022 while the build job uses unity/windows-2019. This difference in Windows versions might lead to inconsistencies or compatibility issues.
Let's check if this is a common pattern across other configurations:
Also applies to: 70-71
✅ Verification successful
Let me gather more context about the purpose of these different Windows versions.
Different Windows versions for test and build jobs appear intentional
The pattern of using Windows 2022 for test jobs and Windows 2019 for build jobs is consistent across both example configurations (personal and pro/plus licenses). Both jobs use the same Unity editor version (2022.3.33f1), and this specific version split appears to be a deliberate choice rather than an inconsistency:
- Test jobs consistently use
unity/windows-2022 - Build jobs consistently use
unity/windows-2019for IL2CPP builds
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Windows executor configurations across all YAML files
rg -A 2 "name: 'unity/windows-" --type yaml
Length of output: 1115
Script:
#!/bin/bash
# Check the job names and their contexts where these Windows versions are used
rg -B 5 -A 2 "name: 'unity/windows-" --type yaml
Length of output: 3088
src/scripts/windows/prepare-env.sh (3)
Line range hint 1-127: Verify secure handling of Unity credentials.
While the script handles sensitive information appropriately through environment variables, let's ensure we're following security best practices.
Let's check for any potential credential leaks:
✅ Verification successful
Credentials are handled securely with no direct exposure in logs
The verification confirms that sensitive Unity credentials (email, password, and serial) are:
- Only accessed via environment variables
- Not exposed in any direct print/echo statements
- Properly passed to Unity CLI through environment variables
- Used within a contained Docker environment
- Only logged in status messages that don't include the actual values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any direct credential usage or logging
rg -i "password|serial|email" --type sh
# Check for any echo or printf statements that might expose credentials
ast-grep --pattern 'printf $_ $UNITY_$_'
ast-grep --pattern 'echo $_ $UNITY_$_'
Length of output: 7324
107-109: Verify environment variable usage across the codebase.
The environment variables have been consistently updated from username to email. Let's ensure this change is reflected across all related files.
Let's check for any remaining references to the old variable names:
✅ Verification successful
Environment variable naming is consistent across the codebase
The environment variable has been consistently updated to use UNITY_EMAIL across all related YAML configuration files. No instances of the old UNITY_USERNAME variable were found, and all configurations are properly aligned with the changes in prepare-env.sh.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining UNITY_USERNAME references that might have been missed
rg -l "UNITY_USERNAME"
# Check for any potential mixed usage of username/email in Docker-related files
fd -e yml -e yaml | xargs rg -l "UNITY_(USERNAME|EMAIL)"
Length of output: 280
Script:
#!/bin/bash
# Check the actual content of these YAML files to see if they use the old or new variable names
for file in src/jobs/test.yml src/jobs/build.yml src/examples/test_and_build_pro_or_plus_license.yml src/examples/test_and_build_personal_license.yml src/commands/return-license.yml src/commands/prepare-env.yml; do
echo "=== $file ==="
rg "UNITY_(USERNAME|EMAIL)" "$file" -A 1 -B 1
done
Length of output: 4886
22-22: LGTM! Verify Unity authentication documentation.
The change from username to email aligns with Unity's authentication system. The error handling and documentation links remain intact.
Let's verify this aligns with Unity's current authentication documentation:
✅ Verification successful
The change from unity_username to unity_email is consistent across the codebase
The codebase shows a complete and consistent transition to using unity_email for Unity authentication across all platforms (Linux, Windows, and macOS) and all relevant scripts. This includes:
- Environment variable definitions in job configurations
- Authentication commands in prepare-env scripts
- License return scripts
- Example configurations
The change aligns with Unity's CLI authentication parameters which use -username for the email parameter.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Unity's documentation or README for authentication references
rg -i "unity.*authentication|unity.*login|unity.*credentials" README.md || echo "No direct references found in README"
Length of output: 156
Script:
#!/bin/bash
# Let's check for Unity-related environment variables or configuration in the codebase
rg -i "unity_(email|username|password|serial)" -A 2 -B 2
# Also check for any documentation files that might contain Unity-related information
fd -e md -e txt -x grep -i -l "unity" {}
Length of output: 22282
src/jobs/build.yml (3)
109-112: LGTM: Parameter references updated consistently
The parameter references in the steps have been correctly updated to use the new unity-email-var-name parameter.
Also applies to: 129-130
30-32:
Remove sensitive command from parameter description
The command for extracting the serial number from a license file should not be exposed in the parameter description.
Instead of exposing the command here, consider:
- Moving it to secure documentation
- Creating a dedicated helper command in the orb to handle license processing safely
- Adding input validation and error handling around the serial number
Required even for personal license.
- TODO: Improve documentation on how to extract from personal license, it's something like this:
- cat Unity_lic.ulf | grep DeveloperData | sed -E 's/.*Value="([^"]+)".*/\1/' | base64 --decode
- For future versions, we could simply request the user to provide the ulf file and we could do the extraction.
+ For personal licenses, please refer to our documentation for instructions on obtaining the serial number.14-16: Verify complete parameter renaming across the codebase
Let's ensure all references to the old parameter name have been updated consistently.
✅ Verification successful
Parameter renaming appears consistent across the codebase
The search results show that all references use the new parameter name unity-email-var-name consistently throughout the codebase. No instances of the old parameter name unity-username were found. The parameter is used consistently in jobs, commands, and example configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old parameter name
# Search for any remaining references to the old parameter names
echo "Checking for remaining references to old parameter names..."
rg -i "unity[_-]username" --type yaml
# Search for any potential mismatches between parameter references
echo "Checking for potential parameter reference mismatches..."
rg -i "unity[_-](email|username)" --type yaml
Length of output: 2480
.circleci/test-deploy.yml (3)
84-86: Authentication parameter changes look good!
The consistent replacement of username-based authentication with email and serial-based authentication across all jobs aligns with the PR's objective of supporting proper Unity activation using serial numbers.
Also applies to: 100-102, 151-153, 166-168, 184-186, 218-220, 236-238, 253-255, 269-271, 287-289, 304-306, 320-322, 355-357, 390-392, 407-409, 426-428, 447-449
Line range hint 87-93: Build configurations are well-structured and consistent
The build configurations maintain consistency across all platforms with appropriate:
- Resource class allocations
- Unity editor version (2022.3.33f1)
- Project paths and build parameters
Also applies to: 188-194, 223-229, 240-246, 257-263, 274-280, 291-297, 308-314, 324-330, 360-366, 395-401, 412-418, 431-437, 452-458
Line range hint 470-497: Publishing workflow requirements correctly updated
The orb publishing workflow has been properly updated to exclude Windows-related jobs from the requirements, while maintaining all other necessary job dependencies.
# Conflicts: # src/scripts/linux/prepare-env.sh
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
src/scripts/linux/prepare-env.sh (2)
63-70: Enhance license file validationThe function only checks for file existence. Consider adding content validation to ensure the license file is properly formatted.
validate_license_file() { + local license_file="/root/.local/share/unity3d/Unity/Unity_lic.ulf" + if [ -e "/root/.local/share/unity3d/Unity/Unity_lic.ulf" ]; then + # Verify file content + if ! grep -q "^<?xml.*<root>.*</root>" "$license_file"; then + echo "Invalid license file format" + return 1 + fi echo "Unity license file successfully generated or applied." return 0 else echo "Failed to generate Unity license file. Please verify your inputs." return 1 fi }
Line range hint
113-116: Use version tags instead of commit hashesUsing a specific commit hash (
173a67e) makes it harder to track which version is being used. Consider using version tags instead.-ref="173a67e" # v3.0.1 +ref="v3.0.1"
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This reverts commit 526072f.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/jobs/test.yml (1)
29-41: Enhance command safety and fix formattingWhile the documentation is comprehensive, consider these improvements:
- The shell command should be properly escaped to handle special characters in filenames
- Remove trailing spaces on lines 30, 37, and 39
Apply this diff to fix the command and trailing spaces:
Required even for personal license. To extract the serial from your Unity personal license file: - + 1. Locate your Unity license file (Unity_lic.ulf) 2. Run the following command to extract the serial: ``` - cat Unity_lic.ulf | grep DeveloperData | sed -E 's/.*Value="([^"]+)".*/\1/' | base64 --decode + cat "${UNITY_LICENSE_FILE:-Unity_lic.ulf}" | grep DeveloperData | sed -E 's/.*Value="([^"]+)".*/\1/' | base64 --decode ``` 3. Store the extracted serial securely in your CI environment variables - + Note: Keep your Unity serial number confidential as it's tied to your Unity account. - + Future Enhancement: We plan to simplify this process by directly accepting the .ulf file and handling the extraction automatically within the orb.🧰 Tools
🪛 yamllint (1.29.0-1)
[error] 30-30: trailing spaces
(trailing-spaces)
[error] 37-37: trailing spaces
(trailing-spaces)
[error] 39-39: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/jobs/test.yml(4 hunks)
🧰 Additional context used
🪛 yamllint (1.29.0-1)
src/jobs/test.yml
[error] 30-30: trailing spaces
(trailing-spaces)
[error] 37-37: trailing spaces
(trailing-spaces)
[error] 39-39: trailing spaces
(trailing-spaces)
🔇 Additional comments (2)
src/jobs/test.yml (2)
79-82: Verify license handling changes
The removal of unity-license-var-name and the switch to unity-serial-var-name suggests a change in the license handling approach. Please ensure that:
- The
prepare-envandreturn-licensecommands have been updated to handle this change - The documentation in related files reflects this new approach
Let's verify the related changes:
Also applies to: 94-95
✅ Verification successful
License handling changes are properly implemented
The verification confirms that:
- Both
prepare-envandreturn-licensecommands have been updated to use the newunity-serial-var-nameparameter consistently - No references to the old
unity-license-var-nameremain in the codebase - The documentation in
return-license.ymlincludes guidance about the serial number requirement and extraction process
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check related command implementations
echo "Checking prepare-env command:"
fd -t f prepare-env.yml -x cat {}
echo -e "\nChecking return-license command:"
fd -t f return-license.yml -x cat {}
# Check for any remaining references to unity-license-var-name
echo -e "\nChecking for old license var references:"
rg 'unity-license-var-name' || echo "No references found"
Length of output: 4609
14-16: Breaking Change: Parameter renamed for clarity
The parameter rename from unity-username-var-name to unity-email-var-name better reflects Unity's authentication mechanism, but this is a breaking change that requires updates in existing workflows.
Let's check for any existing usage that needs updating:
✅ Verification successful
Let me verify if there are any references to the old environment variable name "UNITY_USERNAME" as well, since that's also changing:
Parameter rename is safe to proceed
The verification shows that all references in the codebase are already using the new unity-email-var-name parameter and UNITY_EMAIL environment variable. There are no lingering references to the old unity-username-var-name parameter or UNITY_USERNAME environment variable, indicating this change has been consistently applied throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old parameter name
rg -l 'unity-username-var-name' || echo "No references found"
Length of output: 80
Script:
#!/bin/bash
# Search for any remaining references to the old environment variable name
rg -l 'UNITY_USERNAME' || echo "No references found"
# Also check for any other yml files that might be using this parameter
fd -e yml -x cat {} \; | grep -B2 -A2 'unity.*var.*name' || echo "No parameter usage found"
Length of output: 7898
|
The tests are green 🎉 I had to retry some failed job due to unity activation (I'm getting used to that). I think this is ready to merge. There were some interesting suggestions. I've created followup issues for them because things are kind of easy to break around here. I'd be happy to receive another review frome someone in here before I merge, create a tag, a release and publish this 🙏 edit: LGTM please! 🚀 |
Mainly trying things here. I've updated the CircleCI secrets for this repository to use
UNITY_SERIALto see if we can properly build on different platforms.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
These updates enhance the overall user experience by improving license management and simplifying the setup process.