-
Notifications
You must be signed in to change notification settings - Fork 218
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
Make saveBot required in the InstallationService interface #1253
Comments
hi, @Moh-inc! thanks for submitting this! 😄 I took at look at the
Just wanted to verify if you ran into this error when you were missing an implementation of On my end, I'll reach out to a member of my team internally to see if there's any historical context behind |
Thank you for getting back to me, I do think it's a little misleading especially when it's needed in oauth token rotation and can only be found when testing out the application rather than be more convenient and have it implemented by default. |
Hi @Moh-inc, thank you so much for taking the time to share this feedback and we are sorry for the confusion you've faced when enabling token rotation for your app. Indeed, having the default implementation for saveBot in the interface can lead to inconsistency and confusion with the requisites of other methods. However, modifying this now could bring breaking changes for a large number of existing apps that don't use token rotations. For this reason, we are unable to consider the improvement in the short term. We may revisit this as necessary when releasing the next major version (the major release won't come in the short term though). Thanks again for the feedback. We will keep this issue open to hear from other developers for the future. |
Thank you for taking it into consideration! |
Make
saveBot
required in theInstallationService
interface.I had to read the source code to understand why token rotation was not working and it was because in the
InstallationService
that i built, it didn't implement thesaveBot
which is used in theMultiTeamsAuthorization
for token rotation. Hence it shouldn't have adefault
implementation.Category (place an
x
in each of the[ ]
)Requirements
Please make sure if this topic is specific to this SDK. For general questions/issues about Slack API platform or its server-side, could you submit questions at https://my.slack.com/help/requests/new instead. 🙇
Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you agree to those rules.
The text was updated successfully, but these errors were encountered: