feat(cli): fetch issue by number directly from GitHub (#17)#45
feat(cli): fetch issue by number directly from GitHub (#17)#45mahsumaktas wants to merge 5 commits intosimiligh:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds GitHub issue fetching to the Changes
Sequence DiagramsequenceDiagram
participant User as CLI User
participant Cmd as Process Command
participant Resolver as resolveIssueRepo
participant Auth as Token Resolver
participant GitHub as GitHub API
participant Converter as githubIssueToPipelineIssue
participant Pipeline as Pipeline
User->>Cmd: run `process` with --number
Cmd->>Resolver: derive org/repo from flags or env
Resolver-->>Cmd: org, repo (or error)
Cmd->>Auth: read TRANSFER_TOKEN / GITHUB_TOKEN
Auth-->>Cmd: token (or error)
Cmd->>GitHub: GET /repos/{org}/{repo}/issues/{number} (auth)
GitHub-->>Cmd: GitHub Issue payload
Cmd->>Converter: map GitHub issue -> pipeline.Issue
Converter-->>Cmd: pipeline.Issue
Cmd->>Pipeline: submit pipeline.Issue for processing
Pipeline-->>User: result / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmd/simili/commands/process.go`:
- Around line 139-148: The override logic in the function updating issue
(variables issue, orgName, repoName, issueNum) ignores a slash-formatted --repo
(owner/name); update the block that applies overrides so that when repoName
contains a "/" you parse it (e.g., split on "/" into owner and repo) and set
issue.Org and issue.Repo accordingly, while still letting explicit --org
override the owner; alternatively, if you prefer not to auto-parse, emit a clear
warning via the same logger when --issue is used together with a slash-formatted
--repo to avoid silent ignoring.
| // Apply optional overrides when --issue is used | ||
| if orgName != "" { | ||
| issue.Org = orgName | ||
| } | ||
| if repoName != "" && !strings.Contains(repoName, "/") { | ||
| issue.Repo = repoName | ||
| } | ||
| if issueNum != 0 { | ||
| issue.Number = issueNum | ||
| } |
There was a problem hiding this comment.
--repo owner/name is silently ignored when combined with --issue.
When --issue is provided along with --repo owner/name (containing a slash), neither the org nor repo from the slash-formatted value are applied because:
- Line 140-142 only sets
issue.Orgfrom--org, not from--repo owner/name - Line 143-145 explicitly skips setting
issue.RepowhenrepoNamecontains "/"
This may cause user confusion. Consider either parsing --repo owner/name here as well, or logging a warning when this combination is detected.
Suggested fix to parse `--repo owner/name` consistently
// Apply optional overrides when --issue is used
- if orgName != "" {
- issue.Org = orgName
- }
- if repoName != "" && !strings.Contains(repoName, "/") {
- issue.Repo = repoName
- }
+ if repoName != "" && strings.Contains(repoName, "/") {
+ parts := strings.SplitN(repoName, "/", 2)
+ if len(parts) == 2 {
+ issue.Org = strings.TrimSpace(parts[0])
+ issue.Repo = strings.TrimSpace(parts[1])
+ }
+ } else {
+ if orgName != "" {
+ issue.Org = orgName
+ }
+ if repoName != "" {
+ issue.Repo = repoName
+ }
+ }🤖 Prompt for AI Agents
In `@cmd/simili/commands/process.go` around lines 139 - 148, The override logic in
the function updating issue (variables issue, orgName, repoName, issueNum)
ignores a slash-formatted --repo (owner/name); update the block that applies
overrides so that when repoName contains a "/" you parse it (e.g., split on "/"
into owner and repo) and set issue.Org and issue.Repo accordingly, while still
letting explicit --org override the owner; alternatively, if you prefer not to
auto-parse, emit a clear warning via the same logger when --issue is used
together with a slash-formatted --repo to avoid silent ignoring.
Signed-off-by: Mahsum Aktas <mahsum@mahsumaktas.com>
0835c1e to
eead182
Compare
|
Friendly ping! 👋 This has been ready for review. Let me know if any changes are needed. |
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/simili/commands/process.go`:
- Around line 139-148: When --issue is used together with --repo owner/name the
current check (!strings.Contains(repoName, "/")) silently ignores owner/name;
fix this by detecting and parsing the owner/name form: if repoName contains "/"
split it into owner and repo (e.g., parts := strings.SplitN(repoName, "/", 2))
and set issue.Org and issue.Repo accordingly (overriding orgName only if owner
present), otherwise keep the existing behavior for simple repo names; update the
block that assigns issue.Org and issue.Repo (the variables issue, orgName,
repoName, issueNum in process.go) to perform this parsing and assignment.
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/simili/commands/process.go`:
- Around line 139-148: The current logic silently ignores a slash-formatted
--repo when --issue is used; change the block that handles
repoName/orgName/issue to detect if repoName contains a "/" and split it into
owner and repo (e.g., parts := strings.SplitN(repoName, "/", 2)) then set
issue.Org = parts[0] and issue.Repo = parts[1]; keep the existing behavior where
an explicit orgName flag overrides the owner part (i.e., if orgName != "" use
that instead of parts[0]); optionally emit a warning via the existing logger if
both orgName and a slash-formatted repo are provided to clarify which value
wins.
|
Closing — this has been open for a while and the codebase may have moved on. Happy to reopen or submit a fresh PR if still needed! |
|
Hey @mahsumaktas , Thanks for contributing. I'm reopning the PR for merge in few hours. I have a PR that need to be merged before this. You can work on any issues from our milestone 0.2.0v https://github.com/similigh/simili-bot/milestone/3 here. And also #53 is something I need help with as well. Thank you. |
🧪 E2E Test✅ Bot responded: yes Test repo → gh-simili-bot/simili-e2e-22148588473 Auto-generated by E2E pipeline |
Summary
Implements #17 — adds support for fetching issues directly from GitHub by number, eliminating the need for a local JSON file.
Usage
The existing
--issue <file>path continues to work as before.Changes
cmd/simili/commands/process.go--issueis not provided but--numberis, fetches the issue viagithub.Client.GetIssue()resolveIssueRepo()— resolves org/repo from:--repo owner/name(preferred)--org+--reposeparatelyGITHUB_REPOSITORYenv var fallbackgithubIssueToPipelineIssue()— converts*github.Issuetopipeline.Issuewith proper field mappingEventType: "issues",EventAction: "opened"for directly fetched issuesGITHUB_REPOSITORYparsing (replaced withstrings.SplitN)cmd/simili/commands/process_test.goTestGithubIssueToPipelineIssue— full conversion with all fieldsTestGithubIssueToPipelineIssue_NilIssue— nil safetyValidation
Notes
GITHUB_TOKEN(orTRANSFER_TOKEN) for authentication — same as existing GitHub client usagego-github/v60package--issue <file>usersSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests