-
Notifications
You must be signed in to change notification settings - Fork 127
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
[GH-902]:Added check to uninstall server if not found in KV store #914
Conversation
* [MI-2738]:Added check to uninstall server if not found in KV store * [MI-2738]:Fixed lint errors
Hello @Kshitij-Katiyar, 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. |
…rmost#914 on jira plugin (#36)
server/instances.go
Outdated
func (p *Plugin) disconnectUsers(instance Instance) error { | ||
err := p.userStore.MapUsers(func(user *User) error { | ||
if !user.ConnectedInstances.Contains(instance.GetID()) { | ||
return nil | ||
} | ||
|
||
_, err := p.disconnectUser(instance, user) | ||
if err != nil { | ||
p.infof("UninstallInstance: failed to disconnect user: %v", err) | ||
} | ||
return nil | ||
}) | ||
if err != nil { | ||
return err | ||
} | ||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just putting this here for discussion. Let me know if you think we should change something @Kshitij-Katiyar
This function will always return nil
because the inner function always returns nil
I'm thinking we can pull out the inner function into disconnectUser
, and have this called in a loop on line 192, instead of having a disconnectUsers
function. It seems the pre-existing code block already had this behavior though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mickmister sure I have reverted the changes for disconnectUsers function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Kshitij-Katiyar. The changes LGTM.
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #914 +/- ##
==========================================
- Coverage 30.14% 29.93% -0.21%
==========================================
Files 49 49
Lines 7467 7526 +59
==========================================
+ Hits 2251 2253 +2
- Misses 5027 5084 +57
Partials 189 189
... and 4 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
@AayushChaudhary0001 Would you be open to reviewing this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested this PR for Jira server, working fine. The user is able to uninstall the server instance even if it is not running. Approved
Summary
Issue