Support UPM private registry on windows runner. #665
Conversation
|
|
|
Important Review skippedReview was skipped due to path filters Files ignored due to path filters (1)
You can disable this status message by setting the WalkthroughWalkthroughThe changes introduce new configuration options for handling package management and registry access. Specifically, an optional input parameter Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Action
participant BuildParameters
participant ImageEnvironmentFactory
participant SetupMac
User->>Action: Provide scopedRegistryUrl
Action->>BuildParameters: Set scopedRegistryUrl
BuildParameters->>ImageEnvironmentFactory: Pass scopedRegistryUrl
ImageEnvironmentFactory->>SetupMac: Set SCOPED_REGISTRY_URL
SetupMac->>ImageEnvironmentFactory: Use SCOPED_REGISTRY_URL for setup
Poem
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: 3
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/platforms/windows/build.ps1is excluded by!**/dist/**
Files selected for processing (5)
- action.yml (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/setup-mac.ts (1 hunks)
Additional comments not posted (3)
src/model/image-environment-factory.ts (2)
Line range hint
1-17: Review ofgetEnvVarStringmethod: ApprovedThe method is well-implemented with appropriate checks and formatting for Docker environment variables. It handles special cases and filters out empty or undefined values effectively.
38-39: Addition ofSCOPED_REGISTRY_URLandPRIVATE_REGISTRY_TOKEN: Approved with suggestionsThe addition of these environment variables supports the PR's objective to enhance UPM registry support on Windows runners. Ensure that the values for these variables are securely handled, especially
PRIVATE_REGISTRY_TOKEN, which should not be exposed in logs or error messages.Verification successful
Verification Successful:
PRIVATE_REGISTRY_TOKENis Securely HandledThe
PRIVATE_REGISTRY_TOKENis set and used as expected without being exposed in logs or error messages. The implementation aligns with the security requirements. No further action is needed regarding this concern.
src/model/image-environment-factory.ts: Environment variable setup.src/model/platform-setup/setup-mac.ts: Environment variable assignment.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that PRIVATE_REGISTRY_TOKEN is not exposed in logs or error messages. # Test: Search for PRIVATE_REGISTRY_TOKEN in logs. Expect: No occurrences. rg --type ts -A 5 $'PRIVATE_REGISTRY_TOKEN'Length of output: 1074
action.yml (1)
264-267: Well-documented addition ofscopedRegistryUrl.The new input parameter
scopedRegistryUrlis well-integrated into the action's configuration. It is marked as optional and has a sensible default of an empty string, which ensures backward compatibility. The description is clear and specifies the condition under which this parameter should be used (packageModeis true), which helps in avoiding confusion for the users.
| public scopedRegistryUrl!: string; | ||
| public upmRegistryToken!: string; |
There was a problem hiding this comment.
Addition of scopedRegistryUrl and upmRegistryToken.
The new properties scopedRegistryUrl and upmRegistryToken are added to the BuildParameters class to support enhanced package management capabilities. These properties are correctly declared and integrated into the class's structure. However, it would be beneficial to add documentation comments for these new properties to improve code maintainability and clarity for other developers.
Consider adding documentation comments above these properties:
// URL of the scoped registry used for resolving package dependencies.
public scopedRegistryUrl!: string;
// Token for authenticating with the UPM registry.
public upmRegistryToken!: string;| process.env.PRIVATE_REGISTRY_TOKEN = buildParameters.upmRegistryToken; | ||
| process.env.SCOPED_REGISTRY_URL = buildParameters.scopedRegistryUrl; |
There was a problem hiding this comment.
Ensure secure handling of sensitive environment variables.
The addition of PRIVATE_REGISTRY_TOKEN and SCOPED_REGISTRY_URL to the environment variables is crucial for supporting private registries. However, ensure that these tokens are handled securely, especially since environment variables can be exposed in logs or error messages. Consider masking these values in logs or using more secure storage mechanisms if applicable.
| static get scopedRegistryUrl(): string { | ||
| const input = Input.getInput('scopedRegistryUrl') ?? ''; | ||
|
|
||
| return input !== '' ? input : ''; | ||
| } | ||
|
|
||
| static get upmRegistryToken(): string { | ||
| const input = Input.getInput('UPM_REGISTRY_TOKEN') ?? ''; | ||
|
|
||
| return input !== '' ? input : ''; | ||
| } |
There was a problem hiding this comment.
Review new getter methods for handling registry configurations.
The addition of scopedRegistryUrl and upmRegistryToken methods in the Input class is a positive step towards enhancing configuration flexibility. However, ensure that these methods handle cases where inputs might be absent or malformed. The current implementation defaults to an empty string if the input is not provided, which is a safe default but might lead to runtime errors if not handled properly elsewhere in the code. Consider adding validation or logging to inform when these inputs are not provided.
|
|
Changes
scopedRegistryUrlRelated 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)