feat: support random suffix#5
Conversation
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
alaudabot
left a comment
alaudabot
left a comment
There was a problem hiding this comment.
Review Summary
Reviewed the PR for random suffix support. Found 2 warnings and 3 suggestions - no blocking issues.
Key Changes
- Millisecond timestamp:
yyyyMMddHHmmssSSS - Random 4-char suffix with optional custom override via
--suffixflag - Updated docs and CLI to support the new naming format
Warnings
- Modulo bias in random suffix generation (utils.go:121)
- Regex re-compilation on every call (utils.go:99,130,145)
Suggestions
- Add unit tests for new utility functions
- Improve fallback randomness
- Translate
--suffixflag description in Chinese docs
See inline comments for details.
|
|
||
| randomSuffix := make([]byte, length) | ||
| for i := range rawRandomBytes { | ||
| randomSuffix[i] = shortSuffixAlphabet[int(rawRandomBytes[i])%len(shortSuffixAlphabet)] |
There was a problem hiding this comment.
Warning (performance/uniform-distribution): Modulo bias detected. Using byte % 36 introduces slight statistical bias since 256 values don't divide evenly by 36. Consider rejection sampling or a larger alphabet.
// Alternative approach using rejection sampling:
idx := int(rawRandomBytes[i]) * len(shortSuffixAlphabet) / 256
randomSuffix[i] = shortSuffixAlphabet[idx]| return "" | ||
| } | ||
|
|
||
| safeSuffix := regexp.MustCompile(`[^a-zA-Z0-9_-]`).ReplaceAllString(trimmedSuffix, "") |
There was a problem hiding this comment.
Warning (performance/regex-compilation): Regex is compiled on every call. Consider pre-compiling at package level:
var safeCharsRegex = regexp.MustCompile(`[^a-zA-Z0-9_-]`)| func sanitizeUsername(username string) string { | ||
| // 移除不允许的字符 | ||
| // Remove unsupported characters. | ||
| reg := regexp.MustCompile(`[^a-zA-Z0-9_.-]`) |
There was a problem hiding this comment.
Warning (performance/regex-compilation): Same issue - regex compiled on every call. Pre-compile at package level for better performance.
|
|
||
| // 移除不允许的字符 | ||
| // Remove unsupported characters. | ||
| reg := regexp.MustCompile(`[^a-z0-9_-]`) |
There was a problem hiding this comment.
Warning (performance/regex-compilation): Same issue - regex compiled on every call.
| } | ||
|
|
||
| // generateShortRandomSuffix returns a random lowercase alphanumeric suffix. | ||
| func generateShortRandomSuffix(length int) string { |
There was a problem hiding this comment.
Suggestion (test/missing): Consider adding unit tests for GenerateTemporalSuffix, generateShortRandomSuffix, and normalizeCustomSuffix to ensure timestamp format and random generation work correctly.
|
|
||
| rawRandomBytes := make([]byte, length) | ||
| _, randomErr := rand.Read(rawRandomBytes) | ||
| if randomErr != nil { |
There was a problem hiding this comment.
Suggestion (refactor/fallback): The fallback uses time.Now().UnixNano() which could theoretically collide in high-concurrency. Consider adding a counter or additional entropy in the fallback path.
| @@ -21,6 +21,7 @@ GitLab CLI 支持使用自定义模板来格式化输出结果，让你可以按 | |||
| - `-f, --config`: 输入配置文件（用户、组、项目定义） | |||
| - `-o, --output`: 输出文件路径 | |||
| - `-t, --template`: 模板文件路径（可选） | |||
There was a problem hiding this comment.
Suggestion (docs/consistency): The --suffix flag description is in English while surrounding text is Chinese. Consider adding Chinese translation: --suffix: 可选自定义后缀（在 prefix 模式下替换随机后缀）
🤖 AI Code Review
SummaryThis PR changes the suffix generation from second-level timestamp to millisecond-level timestamp + 4-character random suffix, and adds a new Review Statistics
Critical Issues
None. Warnings
Suggestions
Positive Feedback
ℹ️ About this reviewThis review was automatically generated using the
|
4 participants
将 gitlab-cli 生成的后缀由秒级时间戳改为 毫秒级时间戳 与 4位随机后缀，改动后的用户名长度不会超出 gitlab 限制。
已运行单元测试，全部通过。并在 tektoncd-operator 中执行过
make prepare-gitlab-data，成功。