Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: change to get new tokenless working #440

Closed
wants to merge 2 commits into from
Closed

feat: change to get new tokenless working #440

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d439dce
fix: update update_commit to support tokenless v3
joseph-sentry May 13, 2024
c476580
test: fix tests
joseph-sentry May 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 11 additions & 1 deletion services/repository.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,9 +253,19 @@
# so we append the branch name with the fork slug
branch_name = commit_updates["head"]["branch"]
# TODO: 'slug' is in a `.get` because currently only GitHub returns that info

if commit_updates["head"].get("slug") != commit_updates["base"].get("slug"):
branch_name = commit_updates["head"]["slug"] + ":" + branch_name
# get rid of repo name in head.slug
try:
username = commit_updates["head"]["slug"].split("/")[0]
branch_name = username + ":" + branch_name
except:
log.error(

Check warning on line 263 in services/repository.py

View check run for this annotation

Codecov Notifications / codecov/patch

services/repository.py#L262-L263 

Added lines #L262 - L263 were not covered by tests

Check warning on line 263 in services/repository.py

View check run for this annotation

Codecov - QA / codecov/patch

services/repository.py#L262-L263 

Added lines #L262 - L263 were not covered by tests

Check warning on line 263 in services/repository.py

View check run for this annotation

Codecov Public QA / codecov/patch

services/repository.py#L262-L263 

Added lines #L262 - L263 were not covered by tests

Check warning on line 263 in services/repository.py

View check run for this annotation

Codecov / codecov/patch

services/repository.py#L262-L263 

Added lines #L262 - L263 were not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"Error updating branch name for commit originating from fork",
extra=dict(repoid=commit.repoid, commit=commit.commitid),
)
commit.branch = branch_name

commit.merged = False
else:
possible_branches = await repository_service.get_best_effort_branches(
Expand Down
3 changes: 2 additions & 1 deletion services/tests/test_repository_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -960,6 +960,7 @@ async def test_update_commit_from_provider_info_pull_from_fork(
_report_json=None,
repository=possible_parent_commit.repository,
)
commit.branch = "main"
dbsession.add(possible_parent_commit)
dbsession.add(commit)
dbsession.flush()
Expand Down Expand Up @@ -993,7 +994,7 @@ async def test_update_commit_from_provider_info_pull_from_fork(
assert commit.pullid == 1
assert commit.totals is None
assert commit.report_json == {}
assert commit.branch == f"some-guy/{commit.repository.name}:main"
assert commit.branch == "some-guy:main"
assert commit.parent_commit_id == possible_parent_commit.commitid
assert commit.state == "complete"
assert commit.author is not None
Expand Down