feat: add GitHub component #177
Conversation
YCK1130
force-pushed
the
YCK1130/github-component
branch
from
e1037f4
to
d1aa87e
Compare
YCK1130
requested review from
GeorgeWilliamStrong,
donch1989,
pinglin,
xiaofei-du and
jvallesm
as code owners
There was a problem hiding this comment.
I finished my first review.
It is a big PR.
I did not have too much time to check the GitHub API document in details one by one, which means I may miss some details.
So, please help me do 2 things
- Please let me know if there is something concerning you that you hope others can re-confirm. This is including
- The parts you want to make your code cleaner.
- The parts you hope others to check the doc as well. And, please attach the doc url.
- The parts you are afraid that will increase the code maintenance in the future
- ....
- Please build several pipelines to make sure your Tasks solid & robust locally.
- To avoid the duplicate work, you can save the recipe and to upload d0/staging in the future. (I am not so sure if we support this function now, and I will also check this part)
| "TASK_GET_REVIEW_COMMENTS", | ||
| "TASK_CREATE_REVIEW_COMMENT", | ||
| "TASK_GET_ALL_ISSUES", | ||
| "TASK_GET_ISSUE", |
There was a problem hiding this comment.
Question
"TASK_GET_ALL_PULL_REQUESTS",
"TASK_GET_PULL_REQUEST",
"TASK_GET_ALL_ISSUES",
"TASK_GET_ISSUE",
I am curious about the purpose of the design here.
If we remain multiple one, will it be cleaner in terms of UX / codebase?
There was a problem hiding this comment.
I make tasks to get a specific pr/issue to simplify the response. If the client can only use multiple ones, they may need to use another component to filter out the response they want and make the pipeline more complex.
Moreover, some detailed information may not be provided in the multiple ones' responses. The purpose of build "get all" here is to provide a tool for users to know what pr/issue the repo has if they don't know the pr/issue number.
There was a problem hiding this comment.
Maybe we can combine the two multiple tasks into a "TASK_GET_REPO_INFO" and return all the info users might want to know?
There was a problem hiding this comment.
Maybe we can combine the two multiple tasks into a "TASK_GET_REPO_INFO" and return all the info users might want to know?
I think your original way will be fine!
Aligning the vendors could bring as benefit in terms of workflow use case in my opinion.
It may be too various to know what users want to know in the cases of workflow because every company has different flows even when there are best practices.
There was a problem hiding this comment.
Hi @YCK1130 @chuang8511
Let's use LIST instead of GET_ALL, as it's a more typical name when listing the data.
"TASK_GET_ALL_PULL_REQUESTS"->TASK_LIST_PULL_REQUESTS"TASK_GET_ALL_ISSUES"->TASK_LIST_ISSUES
There was a problem hiding this comment.
Got it!
One more thing, there are page, page_number options to determine how many items would be returned, should I include these in the field?
|
Concerns about this component
|
| "TASK_GET_REVIEW_COMMENTS", | ||
| "TASK_CREATE_REVIEW_COMMENT", | ||
| "TASK_GET_ALL_ISSUES", | ||
| "TASK_GET_ISSUE", |
There was a problem hiding this comment.
Maybe we can combine the two multiple tasks into a "TASK_GET_REPO_INFO" and return all the info users might want to know?
I think your original way will be fine!
Aligning the vendors could bring as benefit in terms of workflow use case in my opinion.
It may be too various to know what users want to know in the cases of workflow because every company has different flows even when there are best practices.
| "TASK_GET_REVIEW_COMMENTS", | ||
| "TASK_CREATE_REVIEW_COMMENT", | ||
| "TASK_GET_ALL_ISSUES", | ||
| "TASK_GET_ISSUE", |
There was a problem hiding this comment.
Hi @YCK1130 @chuang8511
Let's use LIST instead of GET_ALL, as it's a more typical name when listing the data.
"TASK_GET_ALL_PULL_REQUESTS"->TASK_LIST_PULL_REQUESTS"TASK_GET_ALL_ISSUES"->TASK_LIST_ISSUES
| return &base.ExecutionWrapper{Execution: e}, nil | ||
| } | ||
|
|
||
| func (e *execution) fillInDefaultValues(input *structpb.Struct) (*structpb.Struct, error) { |
There was a problem hiding this comment.
@YCK1130 @chuang8511
How about let's move this function to the base package and make it available for all components.
There was a problem hiding this comment.
That is a good idea. However, there are some concerns described in the Slack.
| "github.com/instill-ai/x/errmsg" | ||
| ) | ||
|
|
||
| func middleWare(req string) int { |
There was a problem hiding this comment.
What is the purpose of this function?
There was a problem hiding this comment.
It's for the mock server to throw errors across various tasks.
| type MockIssuesService struct{} | ||
|
|
||
| func (m *MockIssuesService) ListByRepo(ctx context.Context, owner, repo string, opt *github.IssueListByRepoOptions) ([]*github.Issue, *github.Response, error) { | ||
| switch middleWare(owner) { |
There was a problem hiding this comment.
Why can the owner string be converted to an HTTP code?
YCK1130
force-pushed
the
YCK1130/github-component
branch
from
73b2674
to
0b36264
Compare
Because
This commit
Related Issue
instill-ai/instill-core#1025
Todo