mattermost / mattermost-plugin-github

GitHub plugin for Mattermost
Apache License 2.0
157 stars 150 forks source link

Support to configure multiple Github Organizations #707

Closed khareyash05 closed 6 months ago

khareyash05 commented 1 year ago

Summary

Configure support to multiple Github organizations

Ticket Link

Fixes #552

mattermost-build commented 1 year ago

Hello @khareyash05,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

Per the Mattermost Contribution Guide, we need to add you to the list of approved contributors for the Mattermost project.

Please help complete the Mattermost contribution license agreement? Once you have signed the CLA, please comment with /check-cla and confirm that the CLA check is green.

This is a standard procedure for many open source projects.

Please let us know if you have any questions.

We are very happy to have you join our growing community! If you're not yet a member, please consider joining our Contributors community channel to meet other contributors and discuss new opportunities with the core team.

khareyash05 commented 1 year ago

/check-cla

khareyash05 commented 1 year ago

hi @hanzei . I am actually facing a problem in https://github.com/mattermost/mattermost-plugin-github/blob/master/server/plugin/plugin.go#L166 . It expect a single organization name for creating a new Client for graphql. How can this be tackled for multiple orgs?

hanzei commented 12 months ago

Does it make sense to move the org name out of the graphql client and pass the list of orgs as part of GetLHSData?

khareyash05 commented 12 months ago

Will try and let you know! That might help Thanks

khareyash05 commented 12 months ago

Does it make sense to move the org name out of the graphql client and pass the list of orgs as part of GetLHSData?

@hanzei If i understood it correctly, graphql.NewClient expects an organization. How can we tackle that issue? Passing organizations through GetLHSData will work for utility function getLHSData but how will be tackle the NewClient function? Can we consider passing orgs as concatenated string? Through my limited understanding of Go, this was I could think of. Kindly help

mattermost-build commented 11 months ago

This PR has been automatically labelled "stale" because it hasn't had recent activity. A core team member will check in on the status of the PR to help with questions. Thank you for your contribution!

hanzei commented 10 months ago

Sorry for the delay.

If we are passing the org as part of GetLHSData, we can remove org from graphql.Client. It's no longer needed.

codecov-commenter commented 10 months ago

Codecov Report

Attention: 71 lines in your changes are missing coverage. Please review.

Comparison is base (c62ee1c) 15.78% compared to head (969d3d7) 15.79%. Report is 9 commits behind head on master.

Files Patch % Lines
server/plugin/plugin.go 0.00% 29 Missing :warning:
server/plugin/api.go 0.00% 25 Missing :warning:
server/plugin/utils.go 0.00% 8 Missing :warning:
server/plugin/command.go 0.00% 4 Missing :warning:
server/plugin/configuration.go 0.00% 3 Missing :warning:
server/plugin/telemetry.go 0.00% 1 Missing :warning:
server/plugin/webhook.go 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #707 +/- ## ========================================== + Coverage 15.78% 15.79% +0.01% ========================================== Files 15 15 Lines 5574 5779 +205 ========================================== + Hits 880 913 +33 - Misses 4652 4824 +172 Partials 42 42 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mickmister commented 6 months ago

Hi @khareyash05, checking in to see how you're doing. Do you have any questions about this task?

Thanks @khareyash05!!

khareyash05 commented 6 months ago

Hi @khareyash05, checking in to see how you're doing. Do you have any questions about this task?

Thanks @khareyash05!!

Hey @mickmister Not able to understand the work to do. Closing the PR. Thanks.