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

Fix branch agnostic too many relationships error #5590

Merged
merged 1 commit into from
Jan 28, 2025
Merged

Fix branch agnostic too many relationships error #5590

merged 1 commit into from
Jan 28, 2025
+98 −1

Conversation

LucasG0
Copy link
Contributor

@LucasG0 LucasG0 commented Jan 27, 2025

Fixes #5559

@github-actions github-actions bot added the group/backend Issue related to the backend (API Server, Git Agent) label Jan 27, 2025
@LucasG0 LucasG0 marked this pull request as draft January 27, 2025 15:44
@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Jan 27, 2025

CodSpeed Performance Report

Merging #5590 will not alter performance

Comparing lgu-fix-multi-rel-br-agnostic (742f3f7) with stable (7e98238)

Summary

✅ 10 untouched benchmarks

@LucasG0 LucasG0 marked this pull request as ready for review January 27, 2025 17:01
@LucasG0 LucasG0 requested a review from a team January 27, 2025 17:01
gmazoyer
gmazoyer approved these changes Jan 27, 2025
View reviewed changes
backend/infrahub/core/relationship/model.py
@@ -1110,7 +1110,7 @@ async def remove_in_db(
# - Update the existing relationship if we are on the same branch
rel_ids_per_branch = peer_data.rel_ids_per_branch()
if branch.name in rel_ids_per_branch:
await update_relationships_to([str(ri) for ri in rel_ids_per_branch[self.branch.name]], to=remove_at, db=db)
await update_relationships_to([str(ri) for ri in rel_ids_per_branch[branch.name]], to=remove_at, db=db)
Copy link
Contributor

Choose a reason for hiding this comment

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

nasty one

Copy link
Contributor

@ajtmccarty ajtmccarty Jan 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

my bad everyone :shame: :shame: :shame:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's confusing that self.branch ends up being main here

ajtmccarty
ajtmccarty approved these changes Jan 27, 2025
View reviewed changes
Copy link
Contributor

@ajtmccarty ajtmccarty left a comment

Choose a reason for hiding this comment

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

you could probably cover this with a unit test, which would run faster and a failure would be easier to troubleshoot in a unit test than a functional test

@LucasG0
Copy link
Contributor Author

LucasG0 commented Jan 28, 2025

you could probably cover this with a unit test, which would run faster and a failure would be easier to troubleshoot in a unit test than a functional test

I just tried with Node API and couldn't properly reproduce, maybe it should use graphql instead to trigger all the logic involved when updating/fetching a relationship. Merging as is for now

@LucasG0 LucasG0 merged commit f0ebbcc into stable Jan 28, 2025
33 checks passed
@LucasG0 LucasG0 deleted the lgu-fix-multi-rel-br-agnostic branch January 28, 2025 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajtmccarty ajtmccarty ajtmccarty approved these changes

@gmazoyer gmazoyer gmazoyer approved these changes

Assignees
No one assigned
Labels
group/backend Issue related to the backend (API Server, Git Agent)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug: Attempting to change credential for repo cannot be fetched due to multiple relationships
3 participants
@LucasG0 @gmazoyer @ajtmccarty