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.

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

MM-37922 - Add support for Chimera to GitLab plugin #255

Merged
merged 6 commits into from
Sep 29, 2021

Conversation

Szymongib
Copy link
Contributor

Summary

Chimera Proxy is an easier way to connect plugins to external OAuth providers. It provides pre-registered OAuth applications allowing users to skip the app registration step.

Design doc: https://docs.google.com/document/d/1a1AzNwxrqA5bcVonZOVu94b4vV6tT4U7fDIKBwf_aUI/edit

This PR makes it possible to connect GitLab Plugin via Chimera. It introduces:

  • New config option, whether to use pre-registered application (Chimera)
  • Changes validation if connection through Chimera is enabled
  • Creates different OAuth config when using Chimera

Ticket Link

https://mattermost.atlassian.net/browse/MM-37922

@Szymongib Szymongib requested a review from iomodo as a code owner August 17, 2021 09:26
@codecov
Copy link

codecov bot commented Aug 17, 2021

Codecov Report

Merging #255 (31958cf) into master (6755649) will decrease coverage by 0.39%.
The diff coverage is 13.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #255      +/-   ##
==========================================
- Coverage   39.78%   39.38%   -0.40%     
==========================================
  Files          16       16              
  Lines        1757     1795      +38     
==========================================
+ Hits          699      707       +8     
- Misses        984     1011      +27     
- Partials       74       77       +3     
Impacted Files Coverage Δ
server/plugin.go 11.65% <0.00%> (-1.71%) ⬇️
server/configuration.go 32.55% <75.00%> (+17.17%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6755649...31958cf. Read the comment docs.

@Szymongib Szymongib requested a review from hanzei August 17, 2021 09:28
@Szymongib Szymongib added the 2: Dev Review Requires review by a core committer label Aug 17, 2021
Copy link
Contributor

@iomodo iomodo left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @Szymongib

Copy link
Collaborator

@hanzei hanzei left a comment

Choose a reason for hiding this comment

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

Great work 👍

.gitignore Outdated

# IDEs
/.idea
/.vscode
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit pick: Unrelated change

@hanzei hanzei added 3: QA Review Requires review by a QA tester and removed 2: Dev Review Requires review by a core committer labels Aug 18, 2021
@hanzei
Copy link
Collaborator

hanzei commented Aug 18, 2021

@DHaussermann See mattermost/mattermost-plugin-github#467 (comment) of for the tests steps.

@hanzei hanzei added this to the v1.4.0 milestone Aug 18, 2021
@hanzei hanzei added the Awaiting Submitter Action Blocked on the author label Aug 19, 2021
@Szymongib Szymongib removed the Awaiting Submitter Action Blocked on the author label Aug 20, 2021
@jasonblais
Copy link
Contributor

@DHaussermann Kind reminder to help with QA review. Similar to mattermost/mattermost-plugin-github#467, we'd like to get ready to ship the changes ahead of the Oct 13 launch, and would be happy to help where we can.

As an aside, I wonder if Demansol would be equipped to help test the changes here or at mattermost/mattermost-plugin-github#467?

@Szymongib
Copy link
Contributor Author

To align with the implementation of the GitHub plugin and support both v5 and v6 servers I have added a fallback to Chimera env var. For more context see: mattermost/mattermost-plugin-github#467 (comment)

@DHaussermann
Copy link

@Szymongib I have done a round of testing on this and it's looking good 👍

  • Regression tested existing oAuth config
  • Tested authentication using the proxy on a cloud server
  • Tested to ensure the plugin can fail over to the environment variable when no value in the config

The only feedback I have is to add the same sentence to the config option as we have on GitHub
"This setting is intended to be used with Mattermost Cloud instances only."

@Szymongib
Copy link
Contributor Author

@DHaussermann I have aligned the setting text with the GitHub plugin.

Copy link

@DHaussermann DHaussermann left a comment

Choose a reason for hiding this comment

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

Tested and passed

  • Functional testing completed
  • I know see the text change on config as expected

Thanks @Szymongib

LGTM!

@DHaussermann DHaussermann added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Sep 28, 2021
@Szymongib Szymongib merged commit ab70b6b into master Sep 29, 2021
@Szymongib Szymongib deleted the feat/add-support-for-chimera branch September 29, 2021 07:30
@hanzei hanzei added the Docs/Not Needed Does not require documentation label Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request Docs/Not Needed Does not require documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants