Remove confusing warning from FileInstallationStore#1115
Remove confusing warning from FileInstallationStore#1115seratch merged 1 commit intoslackapi:mainfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1115 +/- ##
=======================================
Coverage 85.35% 85.35%
=======================================
Files 110 110
Lines 10517 10517
=======================================
Hits 8977 8977
Misses 1540 1540
Continue to review full report at Codecov.
|
| except FileNotFoundError as e: | ||
| message = f"Failed to find bot installation data for enterprise: {e_id}, team: {t_id}: {e}" | ||
| self.logger.warning(message) | ||
| message = f"Any bot installation data was found for enterprise: {e_id}, team: {t_id}: {e}" |
There was a problem hiding this comment.
Did you mean “No” instead of “Any” here (and in line 175)?
I’m thinking something like:
f”No bot installation data found for enterprise {e_id}, team {t_id}: {e}"
or maybe even better without the negative:
f”Installation data missing for enterprise {e_id}, team {t_id}: {e}"
There was a problem hiding this comment.
Oops, yes, I meant "No" but I like your suggestion. Will update. Thanks!
Refer to slackapi/bolt-python#464 for the context of this change
9e0604a to
1c0c1a8
Compare
|
@eddyg Thanks for your review (as always 🙇)! I've fixed the log messages and removed unintentionally added commits. |
|
You're very welcome, @seratch. (I figured those were extra commits for another PR that snuck in. 😉) |
filmaj
left a comment
There was a problem hiding this comment.
This looks OK to me but I am wondering: aside from the situation described in slackapi/bolt-python#464, perhaps there could be other situations where the bot_filepath might be not found? E.g. wrong permissions set on the directory (or perhaps that throws a different error)?
If so, then perhaps dropping the log level from warning to debug may hide those issues? If not: then please ignore this comment 😄
|
@filmaj Thanks for the comment and that's a good point! But there is no problem with it. In the scenario, bolt-python's authorization middleware writes some error logs and returns an error message to end-users. https://github.com/slackapi/bolt-python/blob/v1.9.1/slack_bolt/middleware/authorization/multi_teams_authorization.py#L69-L76 Thus, the changes in this PR just removes a warning log that exists only in the file-based installation store. |
filmaj
left a comment
There was a problem hiding this comment.
Great, thanks for the details, it is very helpful to learn 🙏
Summary
We've got a question about the warning at slackapi/bolt-python#464 and I found that we should not have the logs in warning level. It's just confusing and also we don't have the same log in warning level in other built-in implementations.
Category (place an
xin each of the[ ])/docs-src(Documents, have you run./scripts/docs.sh?)/docs-src-v2(Documents, have you run./scripts/docs-v2.sh?)/tutorial(PythOnBoardingBot tutorial)tests/integration_tests(Automated tests for this library)Requirements (place an
xin each[ ])python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.shafter making the changes.