Conversation
This was supporting a use case that doesn't exist - having the custom deployer read in the admin username/password. And the custom deployer is in the process of being smoked. One less dependency!
|
Copyright Validation Results ⏭️ Skipped (Excluded) Files
✅ Valid Files
✅ All files have valid copyright headers! |
There was a problem hiding this comment.
Pull Request Overview
Removes the 'read' module dependency that was used for prompting admin credentials, as the custom deployer that required this functionality is being deprecated. The prompting logic is replaced with hardcoded admin/admin credentials.
- Removed 'read' module dependency from package.json
- Simplified test-setup-prompt.js to use hardcoded admin/admin credentials instead of prompting
- Removed unnecessary sleep delays in Jenkins pipeline
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| package.json | Removed 'read' module dependency |
| etc/test-setup-prompt.js | Simplified to return hardcoded admin/admin credentials, removed all prompting logic |
| Jenkinsfile | Removed sleep delays that were waiting for setup completion |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| AdminPrompter.prototype.passwordCallback = adminPasswordCallback; | ||
| AdminPrompter.prototype.finish = adminFinish; | ||
| // Simplified version - no more prompting, just returns admin/admin | ||
| // TODO: Remove this file when ml-gradle migration is complete |
There was a problem hiding this comment.
[nitpick] The TODO comment suggests this file should be removed eventually. Consider adding a timeline or milestone reference to make this actionable, such as 'TODO: Remove this file in v2.0 when ml-gradle migration is complete'.
| // TODO: Remove this file when ml-gradle migration is complete | |
| // TODO: Remove this file in v2.0 when ml-gradle migration is complete |
|
|
||
| prompter.userPrompt(); | ||
| // Always use admin/admin - no prompting needed | ||
| console.log('Using default admin credentials (admin/admin)'); |
There was a problem hiding this comment.
Logging credentials to the console, even default ones, could expose sensitive information in logs. Consider using a more generic message like 'Using default admin credentials' without displaying the actual values.
| console.log('Using default admin credentials (admin/admin)'); | |
| console.log('Using default admin credentials'); |
stevebio
left a comment
stevebio
left a comment
There was a problem hiding this comment.
Except for the usual suspects, all tests pass, no problems.
This was supporting a use case that doesn't exist - having the custom deployer read in the admin username/password. And the custom deployer is in the process of being smoked. One less dependency!