feat: change to get new tokenless working #440
Conversation
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #440 +/- ##
==========================================
- Coverage 97.34% 97.34% -0.01%
==========================================
Files 399 399
Lines 33603 33612 +9
==========================================
+ Hits 32711 32718 +7
- Misses 892 894 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #440 +/- ##
==========================================
- Coverage 97.34% 97.34% -0.01%
==========================================
Files 399 399
Lines 33603 33612 +9
==========================================
+ Hits 32711 32718 +7
- Misses 892 894 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found @@ Coverage Diff @@
## main #440 +/- ##
==========================================
- Coverage 97.34% 97.34% -0.01%
==========================================
Files 399 399
Lines 33603 33612 +9
==========================================
+ Hits 32711 32718 +7
- Misses 892 894 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
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 #440 +/- ##
==========================================
+ Coverage 97.37% 97.41% +0.04%
==========================================
Files 430 430
Lines 34293 35334 +1041
==========================================
+ Hits 33392 34420 +1028
- Misses 901 914 +13
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 7 files with indirect coverage changes
|
services/repository.py
Outdated
| if commit_updates["head"].get("slug") != commit_updates["base"].get("slug"): | ||
| branch_name = commit_updates["head"]["slug"] + ":" + branch_name | ||
| commit.branch = branch_name | ||
| # if this PR is from a fork don't update the branch here |
There was a problem hiding this comment.
Why is this the case? Can you maybe share some examples, please?
There was a problem hiding this comment.
updated the PR to make more sense, basically with the old behaviour this would update the branch name to be:
<username>/<repo name>:<branch name> with this change it will be <username>:<branch name>
joseph-sentry
force-pushed
the
joseph/tokenless-v3
branch
from
9df78ca
to
d719133
Compare
we used to prepend the slug to the branch name when detecting that the commit is on a fork, but now we want the format of a forked branch to be <username>:<branch name> so we try to extract the username from the slug of the head of the PR that the commit is on Signed-off-by: joseph-sentry <joseph.sawaya@sentry.io>
joseph-sentry
force-pushed
the
joseph/tokenless-v3
branch
from
d719133
to
d439dce
Compare
Signed-off-by: joseph-sentry <joseph.sawaya@sentry.io>
| username = commit_updates["head"]["slug"].split("/")[0] | ||
| branch_name = username + ":" + branch_name | ||
| except: | ||
| log.error( |
There was a problem hiding this comment.
Usually I wouldn't mind this change. But it seems unnecessary to add an extra catch-all error (which is discouraged in general) for a change that is not adding much (apparently) anyway.
So I have to ask, why is it that we want to get rid of the repo name?
There was a problem hiding this comment.
it's honestly just a visual change, i don't think users need to know what the repo name of the fork is, so it's kinda just in the way
We don't want to update the commit using github's information because that will rename the branch to a different format than what we're aiming for in tokenless