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-42918] Add debug information for setup wizard #679

Merged
merged 1 commit into from
May 23, 2023

Conversation

hanzei
Copy link
Contributor

@hanzei hanzei commented May 21, 2023

Summary

I'm unable to reproduce the issue linked in the ticket.

To debug the issue if it occurs again, I've added debug information on why exactly the wizard didn't run.

If the PR gets approved, I'm going to copy the change into https://github.com/mattermost/mattermost-plugin-gitlab.

Ticket Link

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

@hanzei hanzei added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels May 21, 2023
@hanzei hanzei added this to the v2.2.0 milestone May 21, 2023
@hanzei hanzei requested a review from mickmister May 21, 2023 18:53
@codecov-commenter
Copy link

Codecov Report

Patch coverage: 59.09% and project coverage change: +0.17 🎉

Comparison is base (64a725b) 15.46% compared to head (25ec692) 15.63%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #679      +/-   ##
==========================================
+ Coverage   15.46%   15.63%   +0.17%     
==========================================
  Files          15       15              
  Lines        5497     5518      +21     
==========================================
+ Hits          850      863      +13     
- Misses       4605     4613       +8     
  Partials       42       42              
Impacted Files Coverage Δ
server/plugin/flows.go 0.00% <0.00%> (ø)
server/plugin/plugin.go 5.92% <0.00%> (-0.06%) ⬇️
server/plugin/utils.go 60.16% <100.00%> (+2.22%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mickmister
Copy link
Member

mickmister commented May 22, 2023

@hanzei What are the requirements to get this merged? Should some particular functionality be tested or regression tested?

@hanzei
Copy link
Contributor Author

hanzei commented May 22, 2023

@mickmister Good question. In order to test this PR, one has to jump through a couple of hoops, namely removing the signature check for the plugin so this build can be installed as part of the onboarding flow.

Once that is done, the PR can be tested. It produces output like this one:
debug [2023-05-22 23:19:30.590 +02:00] OAuth is configured, skipping setup wizard caller="app/plugin_api.go:970" plugin_id=github GitHubOAuthClientID="***8cb3" GitHubOAuthClientSecret="***3d78" UsePreregisteredApplication=false

To me that is enough testing given the complex steps to get there. Are you fine with emerging the PR as it is?

@hanzei
Copy link
Contributor Author

hanzei commented May 22, 2023

GitLab PR: mattermost/mattermost-plugin-gitlab#379

@mickmister
Copy link
Member

To me that is enough testing given the complex steps to get there. Are you fine with emerging the PR as it is?

LGTM 👍

@mickmister mickmister merged commit cbad10e into master May 23, 2023
10 checks passed
@mickmister mickmister deleted the MM-42918_setup-debug branch May 23, 2023 06:18
@hanzei hanzei added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels May 23, 2023
@avas27JTG avas27JTG mentioned this pull request Nov 9, 2023
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants