-
Notifications
You must be signed in to change notification settings - Fork 26
[Feature] add --template-id and --callback-url command line arguments for pipeline integration #51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Summary of ChangesHello @lanmaoxinqing, 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! This pull request significantly enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces --template-id and --callback-url flags to support CI/CD integration. The implementation correctly uses a CITrigger interface to decouple the controller from the trigger logic, which is a great design choice.
I've found a couple of issues:
- A bug in
main.gocauses the CI trigger to always be active, regardless of whether the new flags are provided. - A potential command argument injection vulnerability exists where user-provided data is passed to an external command.
I've provided detailed comments and suggestions to address these points. Otherwise, the changes look good and align with the PR's goal.
| func (cli *ACIHackClient) trigger(name string, version string) error { | ||
| cmd := exec.Command("aci_hack", name, version) | ||
| func (cli *ACIHackClient) trigger(name string, version string, templateId string, callbackURL string) error { | ||
| cmd := exec.Command("aci_hack", name, version, "--template", templateId, "--callback-url", callbackURL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential command argument injection vulnerability. The name and version parameters, which can originate from user-controlled input (e.g., URL path parameters), are passed directly as arguments to an external command. A malicious user could provide values that are interpreted as flags by the aci_hack command (e.g., --help, or other flags that alter its behavior).
While exec.Command prevents shell injection, it does not protect against argument injection. It is highly recommended to validate and sanitize name and version upstream to ensure they do not contain characters that could be misinterpreted by the aci_hack executable. For example, you could enforce that they only contain alphanumeric characters, dashes, and dots.
envhub/main.go
Outdated
| var ciTrigger clients.ACITrigger | ||
| if templateId != "" && callbackURL != "" { | ||
| ciTrigger = clients.ACITrigger{ | ||
| TemplateId: templateId, | ||
| CallbackURL: callbackURL, | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two issues with the current ciTrigger initialization logic:
-
Bug:
ciTriggeris declared as a struct type (clients.ACITrigger). A struct value is nevernil, even if it's a zero-value. When passed toNewEnvController, it's wrapped in aservice.CITriggerinterface, which will also not benil. Consequently, theif ctrl.ciTrigger != nilcheck inEnvControllerwill always pass, and the trigger will be executed even when--template-idand--callback-urlare not provided. To fix this,ciTriggershould be declared as an interface type (service.CITrigger). -
Restrictive Condition: The
&&condition requires both--template-idand--callback-urlto be present. It would be more flexible to use||to enable the trigger if at least one of them is provided, as they might have independent uses.
Here is a suggested fix that addresses both points.
| var ciTrigger clients.ACITrigger | |
| if templateId != "" && callbackURL != "" { | |
| ciTrigger = clients.ACITrigger{ | |
| TemplateId: templateId, | |
| CallbackURL: callbackURL, | |
| } | |
| } | |
| var ciTrigger service.CITrigger | |
| if templateId != "" || callbackURL != "" { | |
| ciTrigger = clients.ACITrigger{ | |
| TemplateId: templateId, | |
| CallbackURL: callbackURL, | |
| } | |
| } |
JacksonMei
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
JacksonMei
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
This PR introduces two new command-line arguments to support future workflow integration and asynchronous notification:
✅ New Flags
--template-id: Specifies the default template ID used for triggering pipelines or environment initialization.--callback-url: Configures a webhook URL to receive asynchronous notifications upon operation completion.🛠 Implementation
--helpoutput, improving CLI usability.📦 Usage Example
🔌 Future Use Cases
templateIdcan be used as the default template for environment creation.CITriggerinterface for customize ci pipelinescallbackURLenables external system integration by triggering callbacks when environment setup or teardown completes.✅ Compatibility