fix(pubisher): support beta and rc GA tags #248
Conversation
Key changes:
Potential problems:
Fixing suggestions:
Overall, the changes look good, but adding more context, refining the regex, and enhancing test coverage would further improve the quality and reliability of the code changes. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @wuhuizuo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request modifies the publisher to correctly support beta and release candidate (rc) GA tags for tiup packages. It introduces a new regular expression ociBetaRCTagRegex to match these tags and updates the transformTiupVer function to handle them appropriately. A new test case is also added to verify the correct transformation of beta tags.
Highlights
- New Regex: Introduces
ociBetaRCTagRegexto match beta and rc GA tags. - Tag Transformation: Updates
transformTiupVerto correctly transform beta and rc GA tags. - Test Case: Adds a new test case to verify the transformation of beta tags.
Changelog
- publisher/pkg/impl/funcs_tiup.go
- Added
ociBetaRCTagRegexto match beta and rc GA tags (line 19). - Modified
transformTiupVerto handle beta and rc GA tags, extracting the version and any additional suffix (lines 166-168).
- Added
- publisher/pkg/impl/funcs_tiup_test.go
- Added a new test case to
Test_transformTiupVerto verify the correct transformation of beta tags (lines 23-30).
- Added a new test case to
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Did you know?
Regular expressions, often shortened to 'regex', originated in the field of theoretical computer science and formal language theory.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request introduces support for beta and rc GA tags in the publisher. The changes look good overall, but I have a few suggestions for improvement.
Merge Readiness
The code changes seem correct and the added test case covers the new functionality. I recommend merging this pull request after addressing the review comments.
| @@ -16,6 +16,7 @@ var ( | |||
| pkgNameRegex = regexp.MustCompile(`^(.+)-v\d+\.\d+\.\d+`) | |||
| pkgVersionNightlyRegex = regexp.MustCompile(`(-\d+-g[0-9a-f]{7,})$`) | |||
| ociGATagRegex = regexp.MustCompile(`^(v\d+\.\d+\.\d+)(-\w+)?_(linux|darwin)_(amd64|arm64)$`) | |||
| ociBetaRCTagRegex = regexp.MustCompile(`^(v\d+\.\d+\.\d+-(beta|rc)\.\d+)?(-\w+)?_(linux|darwin)_(amd64|arm64)$`) | |||
There was a problem hiding this comment.
The regex ociBetaRCTagRegex includes an optional group (v\d+\.\d+\.\d+-(beta|rc)\.\d+)?. However, the subsequent code in transformTiupVer assumes that matches[1] always exists. If the tag doesn't match this optional group, matches[1] will be empty, potentially leading to unexpected behavior or errors. Consider making the entire regex mandatory, or adding a check to ensure matches[1] exists before using it.
Can you confirm that this regex is correct and handles the case where the version part is missing?
| matches := ociGATagRegex.FindStringSubmatch(tag) | ||
| return strings.Join(matches[1:3], "") | ||
| case ociBetaRCTagRegex.MatchString(tag): | ||
| matches := ociBetaRCTagRegex.FindStringSubmatch(tag) | ||
| return strings.Join([]string{matches[1], matches[3]}, "") |
There was a problem hiding this comment.
Consider adding a default case to the switch statement to handle unexpected tag formats. This can prevent unexpected behavior and make the code more robust. For example:
default:
return version // Or log an error and return a default value| { | ||
| name: "test2", | ||
| args: args{ | ||
| version: "v9.0.0-beta.1.pre", | ||
| tag: "v9.0.0-beta.1_linux_amd64", | ||
| }, | ||
| want: "v9.0.0-beta.1", | ||
| }, |
There was a problem hiding this comment.
It would be beneficial to add more test cases to cover different scenarios for beta and rc tags, including cases with and without suffixes, and different versions. This will ensure the regex and transformation logic are robust and handle various tag formats correctly.
For example, consider adding test cases for:
v9.0.0-rc.1_linux_amd64v9.0.0-beta.1-centos7_linux_amd64v9.0.0-rc.1-centos7_linux_amd64
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wuhuizuo The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
No description provided.