Skip to content
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.

Sign up for GitHub

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

Jump to bottom

refactor: add subtask register for github plugin #5411

Merged
merged 31 commits into from
Jul 20, 2023
Merged

refactor: add subtask register for github plugin #5411

merged 31 commits into from
Jul 20, 2023

Conversation

chenggui53
Copy link
Contributor

@chenggui53 chenggui53 commented Jun 8, 2023 •
edited

⚠️ Pre Checklist

Please complete ALL items in this checklist, and remove before submitting

  • I have read through the Contributing Documentation.
  • I have added relevant tests.
  • I have added relevant documentation.
  • I will add labels to the PR, such as pr-type/bug-fix, pr-type/feature-development, etc.

Summary

What does this PR do?
like pr #4896, add subtask register for github

Does this close any open issues?

Closes #4413

Screenshots

Include any relevant screenshots here.

Other Information

Any other information that is important to this PR.

@chenggui53 chenggui53 force-pushed the add-subtask-register-all branch 2 times, most recently from e6505bb to 9efb91f Compare June 9, 2023 01:41
@chenggui53 chenggui53 requested a review from klesh June 9, 2023 01:42
@chenggui53
Copy link
Contributor Author

Hi @klesh, Should i create pr for each plugin to add this register, or do this in one pr ?

klesh
klesh reviewed Jun 9, 2023
View reviewed changes
backend/plugins/github/tasks/pr_commit_collector.go Outdated
@@ -44,6 +48,7 @@ var CollectApiPullRequestCommitsMeta = plugin.SubTaskMeta{
EnabledByDefault: true,
Description: "Collect PullRequestCommits data from Github api, supports both timeFilter and diffSync.",
DomainTypes: []string{plugin.DOMAIN_TYPE_CROSS, plugin.DOMAIN_TYPE_CODE_REVIEW},
Dependencies: []*plugin.SubTaskMeta{&ExtractApiEventsMeta},
Copy link
Contributor

Choose a reason for hiding this comment

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

I just realize that the Dependencies are defined based on the current order which would work but is not accurate.
For example, the CollectApiPullRequestCommitsMeta should depend on the ExtractApiPullRequestMete instead of ExtractApiEventsMeta.
Would you mind updating the code to reflect that? The trick is to identify which table is required in the collector as the Input parameter, and find out the Extractor of the table.

@klesh
Copy link
Contributor

klesh commented Jun 9, 2023

Hi, @chenggui53 , thanks for asking. I think both are fine for this refactor.
I saw the dependencies are deduced from the Hardcoded Order which is not accurate, would you mind fixing them as well? Maybe we should do it One by One so I can verify the dependency for you.

BTW, Your contribution means a lot to the project, would you like to join us as an Apache Committer?

@chenggui53
Copy link
Contributor Author

chenggui53 commented Jun 9, 2023 •
edited

Hi, @chenggui53 , thanks for asking. I think both are fine for this refactor. I saw the dependencies are deduced from the Hardcoded Order which is not accurate, would you mind fixing them as well? Maybe we should do it One by One so I can verify the dependency for you.

BTW, Your contribution means a lot to the project, would you like to join us as an Apache Committer?

@klesh Your comments are very helpful, this register is a step forward, I will fix this part as soon as possible.
And thank you very much for your invitation, I am very happy to be a committer of this project

@chenggui53 chenggui53 force-pushed the add-subtask-register-all branch 2 times, most recently from 2c44c4a to c570292 Compare June 9, 2023 09:00
@klesh
Copy link
Contributor

klesh commented Jun 9, 2023

Hi, @chenggui53 , thanks for asking. I think both are fine for this refactor. I saw the dependencies are deduced from the Hardcoded Order which is not accurate, would you mind fixing them as well? Maybe we should do it One by One so I can verify the dependency for you.
BTW, Your contribution means a lot to the project, would you like to join us as an Apache Committer?

@klesh Your comments are very helpful, this register is a step forward, I will fix this part as soon as possible. And thank you very much for your invitation, I am very happy to be a committer of this project

Nice, I will nominate you ASAP, it may take a couple of weeks.

@chenggui53 chenggui53 force-pushed the add-subtask-register-all branch 9 times, most recently from a67bbfd to a4aa5e0 Compare June 13, 2023 10:57
@klesh
Copy link
Contributor

klesh commented Jun 15, 2023

@chenggui53 I thought we agree on "One PR for Plugin" so I can help verify the dependencies are correct. Seems like this PR contains multiple plugins.

@chenggui53
Copy link
Contributor Author

chenggui53 commented Jun 15, 2023 •
edited

@chenggui53 I thought we agree on "One PR for Plugin" so I can help verify the dependencies are correct. Seems like this PR contains multiple plugins.

@klesh sorry, i've modified multiple plugins in this branch for test，it's not ready for review. i will create a new branch for test and request your review as it done

@chenggui53 chenggui53 force-pushed the add-subtask-register-all branch 2 times, most recently from 0e2daef to 3e3eda0 Compare June 22, 2023 10:06
@chenggui53 chenggui53 closed this Jun 23, 2023
@chenggui53 chenggui53 deleted the add-subtask-register-all branch June 23, 2023 16:32
@chenggui53 chenggui53 reopened this Jun 23, 2023
@chenggui53 chenggui53 force-pushed the add-subtask-register-all branch 2 times, most recently from c1b138d to 604d771 Compare June 24, 2023 15:01
@chenggui53
refactor: add subtask register
5f1568e
@chenggui53
refactor: add subtask register
ba0c96f
@chenggui53
refactor: add subtask register
1d2f614
@chenggui53
refactor: add subtask register
044943d
@chenggui53
refactor: add subtask register
2422bb5
@chenggui53
refactor: add subtask register
5d6b670
@chenggui53
refactor: add subtask register
9a462c0
@chenggui53
refactor: add subtask register
6788fca
@chenggui53
refactor: add subtask meta register
e9e4c33
@chenggui53
refactor: add subtask register
675a9ea
@chenggui53
refactor: add subtask register
c77cc19
@chenggui53
refactor: add subtask register
eff7c22
@chenggui53
refactor: Update remote.go
9c40e0a
@chenggui53
refactor: add subtask register
21f6206
@chenggui53
refactor: add subtask register
f51b4a2
@chenggui53
refactor: add subtask register
e489477
@chenggui53
refactor: add subtask register
658ce99
@chenggui53
refactor: add subtask register
bc460a9
@chenggui53
refactor: update subtask register
10c7fe7
@chenggui53
refactor: update subtask register
844d80f
@chenggui53
refactor: add subtask register
e98293e
@chenggui53
refactor: add subtask register
54aee04
@chenggui53
refactor: add subtask register
416fcbd
@klesh
Copy link
Contributor

klesh commented Jul 20, 2023

@chenggui53 Seems ready to merge, am I right?

@chenggui53
Copy link
Contributor Author

@chenggui53 Seems ready to merge, am I right?

@klesh Yes, I think so.

klesh
klesh approved these changes Jul 20, 2023
View reviewed changes
Copy link
Contributor

@klesh klesh left a comment

Choose a reason for hiding this comment

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

LGTM

@klesh klesh merged commit cc39ef2 into apache:main Jul 20, 2023
9 checks passed
d4x1 pushed a commit to merico-dev/lake that referenced this pull request Jul 21, 2023
@chenggui53 @d4x1
refactor: add subtask register for github plugin (apache#5411)
36b3b5d 
* refactor: add subtask registert for github

* refactor: add subtask register

* refactor: add subtask register

* refactor: add subtask register

* refactor: add subtask register

* refactor: add subtask register

* refactor: add subtask register

* refactor: add subtask register

* refactor: add subtask register

* refactor: add subtask register

* refactor: add subtask register

* refactor: add subtask register

* refactor: add subtask register

* refactor: add subtask register

* refactor: add subtask register

* refactor: add subtask register

* refactor: add subtask meta register

* refactor: add subtask register

* refactor: add subtask register

* refactor: add subtask register

* refactor: Update remote.go

* refactor: add subtask register

* refactor: add subtask register

* refactor: add subtask register

* refactor: add subtask register

* refactor: add subtask register

* refactor: update subtask register

* refactor: update subtask register

* refactor: add subtask register

* refactor: add subtask register

* refactor: add subtask register
@chenggui53 chenggui53 mentioned this pull request Jul 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keon94 keon94 keon94 left review comments

@klesh klesh klesh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@chenggui53 @klesh @keon94