Skip to content
This repository has been archived by the owner on Oct 29, 2024. It is now read-only.

feat: add GitHub component #177

Merged
merged 38 commits into from
Jul 11, 2024
Merged

feat: add GitHub component #177

merged 38 commits into from
Jul 11, 2024

Conversation

YCK1130
Copy link
Collaborator

@YCK1130 YCK1130 commented Jun 25, 2024 •
edited
Loading

Because

  • We need a GitHub component to complete some development automation tasks

This commit

Related Issue

instill-ai/instill-core#1025

Todo

  • TASK_GET_ALL_PULL_REQUESTS: function to get all prs given owner name and repository name.
  • TASK_GET_PULL_REQUEST: function to get a specific pr given owner name, repository name, and pr number. (including file changes)
  • TASK_GET_REVIEW_COMMENT: get review comment inside a pull request
  • TASK_CREATE_REVIEW_COMMENT: create review comment inside a pull request
  • TASK_GET_COMMIT: get commit messages and file changes
  • TASK_CREATE_ISSUE: post issue
  • TASK_GET_ALL_ISSUES: get all issues in a repo
  • TASK_GET_ISSUE: get an issue
  • TASK_CREATE_WEBHOOK: register webhook, https://docs.github.com/en/webhooks/webhook-events-and-payloads

@YCK1130 YCK1130 marked this pull request as ready for review July 1, 2024 15:42
GeorgeWilliamStrong
GeorgeWilliamStrong suggested changes Jul 2, 2024
View reviewed changes
Copy link
Contributor

@GeorgeWilliamStrong GeorgeWilliamStrong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job with the documentation, just left a few minor comments for you to address.

chuang8511
chuang8511 reviewed Jul 3, 2024
View reviewed changes
Copy link
Member

@chuang8511 chuang8511 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. 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
  • ....
  1. 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)

application/github/v0/config/definition.json Outdated Show resolved Hide resolved
application/github/v0/config/definition.json
"TASK_GET_REVIEW_COMMENTS",
"TASK_CREATE_REVIEW_COMMENT",
"TASK_GET_ALL_ISSUES",
"TASK_GET_ISSUE",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can combine the two multiple tasks into a "TASK_GET_REPO_INFO" and return all the info users might want to know?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. I think we should add it. You can open another PR to include it later.

application/github/v0/config/tasks.json Outdated Show resolved Hide resolved
application/github/v0/config/tasks.json Show resolved Hide resolved
application/github/v0/config/tasks.json Outdated Show resolved Hide resolved
application/github/v0/config/tasks.json Show resolved Hide resolved
application/github/v0/commits.go Show resolved Hide resolved
application/github/v0/commits.go Outdated Show resolved Hide resolved
application/github/v0/pull_request.go Outdated Show resolved Hide resolved
application/github/v0/pull_request.go Outdated Show resolved Hide resolved
@YCK1130
Copy link
Collaborator Author

YCK1130 commented Jul 4, 2024

Concerns about this component

  1. The rate limit of GitHub API calls for unauthenticated requests is 60 requests per hour, which is quite small. I left the token field optional because some tasks are available without authentication.
  • Personal access token rate limit: 5000/hr
  • GitHub Enterprise Cloud organization: 15000/hr
  1. The "create review comment" task may need more checks. The docs of GitHub are quite unclear and there seems to be some mismatch between the document and the actual API call.
  2. I am using a Mock server to simulate the API calls just like in other components. I don't know whether it will be difficult to maintain in the future.

chuang8511
chuang8511 reviewed Jul 5, 2024
View reviewed changes
Copy link
Member

@chuang8511 chuang8511 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

second draft review.
I will check in details next week.

application/github/v0/config/definition.json
"TASK_GET_REVIEW_COMMENTS",
"TASK_CREATE_REVIEW_COMMENT",
"TASK_GET_ALL_ISSUES",
"TASK_GET_ISSUE",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

application/github/v0/config/tasks.json Outdated Show resolved Hide resolved
application/github/v0/main.go Outdated Show resolved Hide resolved
donch1989
donch1989 suggested changes Jul 8, 2024
View reviewed changes
application/github/v0/config/definition.json
"TASK_GET_REVIEW_COMMENTS",
"TASK_CREATE_REVIEW_COMMENT",
"TASK_GET_ALL_ISSUES",
"TASK_GET_ISSUE",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

application/github/v0/main.go
return &base.ExecutionWrapper{Execution: e}, nil
}

func (e *execution) fillInDefaultValues(input *structpb.Struct) (*structpb.Struct, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@YCK1130 @chuang8511
How about let's move this function to the base package and make it available for all components.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a good idea. However, there are some concerns described in the Slack.

application/github/v0/client.go Show resolved Hide resolved
application/github/v0/issues.go Outdated Show resolved Hide resolved
application/github/v0/issues.go Outdated Show resolved Hide resolved
application/github/v0/review_coment.go Outdated Show resolved Hide resolved
application/github/v0/utils.go
"github.com/instill-ai/x/errmsg"
)

func middleWare(req string) int {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of this function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's for the mock server to throw errors across various tasks.

application/github/v0/issue_service_test.go
type MockIssuesService struct{}

func (m *MockIssuesService) ListByRepo(ctx context.Context, owner, repo string, opt *github.IssueListByRepoOptions) ([]*github.Issue, *github.Response, error) {
switch middleWare(owner) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can the owner string be converted to an HTTP code?

donch1989
donch1989 suggested changes Jul 11, 2024
View reviewed changes
Copy link
Member

@donch1989 donch1989 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update all property keys to kebab-case.

application/github/v0/config/tasks.json Outdated Show resolved Hide resolved
application/github/v0/config/tasks.json Outdated Show resolved Hide resolved
application/github/v0/config/tasks.json Outdated Show resolved Hide resolved
@YCK1130
chore: update readme
5816908
@YCK1130
chore: update readme
95a57ca
@YCK1130
chore: update readme
4832928
@YCK1130
chore: maintain consistency in readme
6f1c7c9
@YCK1130
feat: add default fill in function
6d13702
@YCK1130
chore: add error handling for review related task
ceca96c
@YCK1130
fix: fix testing case
dba9763
@YCK1130
fix: move component to application
18f8f0e
@YCK1130
fix: fix api consistency of get a pr
1f56891
@YCK1130
fix: format files
6674210
@YCK1130
chore: update readme
4a061bd
@YCK1130
fix: make repo info to component struct
a80445a
@YCK1130
fix: change task name
a0f6132
@YCK1130
fix: refactor client struct
bb9b70c
@YCK1130
fix: pass context from Execute
b34007c
@YCK1130
fix: update task name
12306c8
@YCK1130
fix: add test case and fix repo info bug
fc8da71
@YCK1130
chore: clarify description
31b9e38
@YCK1130
chore: remove confusing output in list pr
08a2605
@YCK1130
fix: patch instillFormat field
43cff96
@YCK1130
chore: flatten the output of single output tasks
a880e8e
@YCK1130
fix: change all params to kebab-case
1525b9d
@YCK1130
fix: update description
ab33a01
@YCK1130
fix: format go files
6d9bdca
@donch1989 donch1989 merged commit 46e5a8e into instill-ai:main Jul 11, 2024
6 checks passed
@droplet-bot droplet-bot mentioned this pull request Jul 11, 2024
Merged
AmeliaCelline pushed a commit to AmeliaCelline/fork-component that referenced this pull request Jul 15, 2024
@YCK1130 @AmeliaCelline
feat: add GitHub component (instill-ai#177)
107e6e0 
- We need a GitHub component to complete some development automation
tasks

instill-ai/instill-core#1025

- [X] TASK_GET_ALL_PULL_REQUESTS: function to get all prs given owner
name and repository name.
- [X] TASK_GET_PULL_REQUEST: function to get a specific pr given owner
name, repository name, and pr number. (including file changes)
- [X] TASK_GET_REVIEW_COMMENT: get review comment inside a pull request
- [X] TASK_CREATE_REVIEW_COMMENT: create review comment inside a pull
request
- [X] TASK_GET_COMMIT: get commit messages and file changes
- [x] TASK_CREATE_ISSUE: post issue
- [x] TASK_GET_ALL_ISSUES: get all issues in a repo
- [x] TASK_GET_ISSUE: get an issue
- [x] TASK_CREATE_WEBHOOK: register webhook,
https://docs.github.com/en/webhooks/webhook-events-and-payloads
donch1989 pushed a commit that referenced this pull request Jul 16, 2024
@droplet-bot
chore(main): release 0.22.0-beta (#196)
1bc00ef 
🤖 I have created a release *beep* *boop*
---


##
[0.22.0-beta](v0.21.0-beta...v0.22.0-beta)
(2024-07-16)


### Features

* add GitHub component
([#177](#177))
([46e5a8e](46e5a8e))
* add JQ input field that accepts any type
([#201](#201))
([cba4aac](cba4aac))
* **cohere:** add Cohere component
([#187](#187))
([63fd578](63fd578))
* **cohere:** add cohere to be able to use instill credit
([#213](#213))
([80415b1](80415b1))
* GitHub component pagination
([#212](#212))
([4b8bbc7](4b8bbc7))
* **instill:** send requester UID, if present, on model trigger
([#202](#202))
([31422cd](31422cd))
* **mistral:** add Mistral AI component
([#204](#204))
([12aaf4f](12aaf4f))
* **openai:** add dimensions in openai component
([#200](#200))
([0d08912](0d08912))
* **text:** add input and output and fix bugs
([#209](#209))
([56ab3eb](56ab3eb))
* unify pipeline and component usage handlers
([#197](#197))
([e27e46c](e27e46c))


### Bug Fixes

* fix instillUpstreamTypes not correctly render the JSON schema
([#216](#216))
([bb603bd](bb603bd))
* **mistralai:** svg naming is wrong
([#218](#218))
([108817a](108817a))
* **text:** hotfix the bug from langchaingo without importing the
function o… ([#217](#217))
([4cfc263](4cfc263))
* typo ([#195](#195))
([d6b2a42](d6b2a42))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
@chuang8511 chuang8511 mentioned this pull request Jul 19, 2024
Merged
7 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@chuang8511 chuang8511 chuang8511 left review comments

@GeorgeWilliamStrong GeorgeWilliamStrong GeorgeWilliamStrong requested changes

@donch1989 donch1989 donch1989 requested changes

@pinglin pinglin Awaiting requested review from pinglin pinglin is a code owner

@xiaofei-du xiaofei-du Awaiting requested review from xiaofei-du xiaofei-du is a code owner

@jvallesm jvallesm Awaiting requested review from jvallesm jvallesm is a code owner

Assignees
No one assigned
Labels
instill component
Projects
No open projects
🔮 Instill Core
Status: 👋 Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@YCK1130 @donch1989 @GeorgeWilliamStrong @chuang8511 @droplet-bot