chore: re-work public tokens (part I) #494
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #494 +/- ##
==========================================
- Coverage 97.26% 97.26% -0.01%
==========================================
Files 412 414 +2
Lines 34393 34479 +86
==========================================
+ Hits 33453 33536 +83
- Misses 940 943 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #494 +/- ##
==========================================
- Coverage 97.26% 97.26% -0.01%
==========================================
Files 412 414 +2
Lines 34393 34479 +86
==========================================
+ Hits 33453 33536 +83
- Misses 940 943 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found @@ Coverage Diff @@
## main #494 +/- ##
==========================================
- Coverage 97.26% 97.26% -0.01%
==========================================
Files 412 414 +2
Lines 34393 34479 +86
==========================================
+ Hits 33453 33536 +83
- Misses 940 943 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Changes have been made to critical files, which contain lines commonly executed in production. Learn more ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #494 +/- ##
==========================================
- Coverage 97.29% 97.29% -0.01%
==========================================
Files 443 445 +2
Lines 35122 35380 +258
==========================================
+ Hits 34172 34423 +251
- Misses 950 957 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 5 files with indirect coverage changes This change has been scanned for critical changes. Learn more |
ticket: https://l.codecov.dev/QLcMzU 1st part of the move to having more dedicated-tokens for certain functionality in the app. It also now allows dedicated-tokens to come from apps (for GitHub)
giovanni-guidini
force-pushed
the
gio/map-dedicated-apps-to-tokentype
branch
from
60bb051
to
40d638a
Compare
services/bots/public_bots.py
Outdated
| TokenType.admin: admin_bot_token, | ||
| # [GitHub] Only legacy PATs can post statuses and comment to all public repos, so there can't be a dedicated_app for this |
There was a problem hiding this comment.
What is "PAT"? Googling the acronym gave me a lot of results. :p
There was a problem hiding this comment.
services/bots/helpers.py
Outdated
| service: str, token_type: TokenType | ||
| ) -> Token | None: | ||
| # GitHub can have 'dedicated_apps', and those are preferred | ||
| dedicated_app = get_config(service, "dedicated_apps", token_type.value, default={}) |
There was a problem hiding this comment.
dedicated_app object looks like it's a dict[str, Any] type. I think we should annotate it for clarity.
| @@ -0,0 +1,32 @@ | |||
| from shared.config import get_config | |||
There was a problem hiding this comment.
It took me a while to realize that get_config was actually loading from YAML configurations. from shared.config import get_config just looked a lot like it's loading application configs, not YAML (I had to dig into the code to verify that it's actually YAML). Anyways, not sure if it's a me thing, or if other people will also be confused.
There was a problem hiding this comment.
from shared.config import get_config just looked a lot like it's loading application configs
That is the case. The fact that the config lives in a YAML is just an implementation detail. For all intents and purposes it's fine to look at get_config as "getting application configs" and understand that the config logically is a map
| pem_path=f"yaml+file://{service}.dedicated_apps.{token_type.value}", | ||
| ) | ||
| return Token(key=actual_token, username=f"{token_type.value}_dedicated_app") | ||
|
|
There was a problem hiding this comment.
I know it's implicit (by not having a return statement) that we return None if we don't go through the if statement, but I think it'll be more readable if we have an explicit return None here.
services/bots/tests/test_helpers.py
Outdated
| # TokenType.read has a dedicated app AND bot. | ||
| # Dedicated app is preferred. |
There was a problem hiding this comment.
This comment will be even more useful as a docstring on the get_token_type_from_config function.
Also, for purposes of test clarity and documentation maintenance, how do you feel about turning each of these asserts into their own parametrize tests (with id annotation). If we fail this test here, we will only be notified for the first test that failed (and you'll have to rerun to get other failures/passes). It'll be more convenient to have the test status for all these checks run regardless of which one failed, especially since each of these asserts seem to be testing different usescases.
services/bots/tests/test_helpers.py
Outdated
| ): | ||
| mock_configuration.set_params( | ||
| { | ||
| "github": { |
There was a problem hiding this comment.
This looks almost duplicated from the params set in the next test. Can we turn this dict into a fixture? (docs for fixtures)
ticket: https://l.codecov.dev/QLcMzU
1st part of the move to having more dedicated-tokens for certain functionality in the app.
It also now allows dedicated-tokens to come from apps (for GitHub)
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.