RFC: Community plugins #5610
RFC: Community plugins #5610
Conversation
Signed-off-by: davidmirror-ops <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5610 +/- ##
==========================================
+ Coverage 35.90% 36.30% +0.40%
==========================================
Files 1301 1305 +4
Lines 109419 110019 +600
==========================================
+ Hits 39287 39943 +656
+ Misses 66035 65920 -115
- Partials 4097 4156 +59
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: davidmirror-ops <[email protected]>
rfc/system/0008-community-plugins.md
Outdated
|
|
||
| ## 6 Open questions | ||
|
|
||
| - Does this apply to Agents? |
There was a problem hiding this comment.
it should. many people contribute agents as well
rfc/system/0008-community-plugins.md
Outdated
|
|
||
| - There's a management overhead associated with this initiative: | ||
| - For every new plugin contributor, we may need to update `CODEOWNERS` | ||
| - CI configuration changes in flytekit (probably a one-time change) |
There was a problem hiding this comment.
we'll need to rearrange a few things, but yeah, definitely a one-time small change.
Signed-off-by: davidmirror-ops <[email protected]>
rfc/system/0008-community-plugins.md
Outdated
| - Create a `community` folder under `flytekit/plugins` and keep releasing the plugins in that folder as separate `pypi` packages. | ||
| - Configure CI to only run tests on `plugins/community` when there are changes. | ||
| - Keep releasing plugins alongside flytekit. | ||
| - Update [CODEOWNERS](https://github.com/flyteorg/flytekit/blob/master/CODEOWNERS) for the `plugins/community` folder, enabling contributors to merge community plugins (it requires granting Write permissions to contributors on `flytekit`). |
There was a problem hiding this comment.
| - Update [CODEOWNERS](https://github.com/flyteorg/flytekit/blob/master/CODEOWNERS) for the `plugins/community` folder, enabling contributors to merge community plugins (it requires granting Write permissions to contributors on `flytekit`). | |
| - Update [CODEOWNERS](https://github.com/flyteorg/flytekit/blob/master/CODEOWNERS) for the `plugins/community` folder so that plugin authors are tagged for PR reviews. | |
| - Enable contributors to merge community plugins (it requires granting Write permissions to contributors on `flytekit`). |
There was a problem hiding this comment.
Let's talk about this point today. Will every plugin author automatically get the permission to approve and merge PRs in the entire repo? Github's permissions model is a bit limiting for us here.
There was a problem hiding this comment.
Revisiting this: how the contribution process would look like? Shouldn't be the existing CODEOWNERS (a.k.a. flytekit maintainers) who review and merge a new community plugin? In such case, we wouldn't need to update CODEOWNERS for every new plugin contribution.
Otherwise with the Github permission model, any new plugin contributor would have to be granted write permissions to the entire flytekit repo.
cc @eapolinario
There was a problem hiding this comment.
Shouldn't be the existing CODEOWNERS (a.k.a. flytekit maintainers) who review and merge a new community plugin?
Yes, a new plugin should be merged by existing maintainers. The question for me is rather what happens after a plugin has been merged and needs to be maintained by the authors.
There was a problem hiding this comment.
What we talked with Eduardo today is that, considering the implication of CODEOWNERS, we would go the regular contribution path: plugin authors would have non-binding approval rights on changes to their plugins, and flytekit maintainers issue the final approvals and merge. We'd also keep releasing community plugins with flytekit, even if there are no changes. Does that make sense?
rfc/system/0008-community-plugins.md
Outdated
|
|
||
| - Does this apply to Agents? |
There was a problem hiding this comment.
| - Does this apply to Agents? | |
| - |
Signed-off-by: Fabio M. Graetz, Ph.D. <[email protected]>
Signed-off-by: davidmirror-ops <[email protected]>
|
Contributors sync notes: @davidmirror-ops to explore if |
Signed-off-by: Fabio M. Graetz, Ph.D. <[email protected]>
Signed-off-by: Fabio M. Graetz, Ph.D. <[email protected]>
Signed-off-by: Fabio M. Graetz, Ph.D. <[email protected]>
There was a problem hiding this comment.
@davidmirror-ops do you agree with merging this and this proposal? Then it LGTM.
Co-authored-by: Fabio M. Graetz, Ph.D. <[email protected]> Signed-off-by: David Espejo <[email protected]>
Tracking issue
Why are the changes needed?
What changes were proposed in this pull request?
How was this patch tested?
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link