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.

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

fix: Only replace projects covered by token #466

Merged
merged 4 commits into from
May 22, 2024

Conversation

chriswk
Copy link
Contributor

@chriswk chriswk commented May 22, 2024

Previously, if the update was empty, we assumed we needed to replace all stored features. This is not the case. We only need to remove the projects that are connected to the token we're using for the merge.

In addition, debug output on /internal-backstage/tokens now includes how many features was received in the update

Previously, if the update was empty, we assumed we needed to replace all
stored features. This is not the case. We only need to remove the
projects that are connected to the token we're using for the merge.

In addition, debug output on `/internal-backstage/tokens` now includes how many features was received in
the update
@chriswk chriswk requested review from nunogois, thomasheartman and sighphyre and removed request for thomasheartman May 22, 2024 10:21
})
.cloned()
.collect();
to_keep.extend(updated.iter().cloned());
Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming that, in case updated is empty, this will act as:
to_keep.extend([]) effectively returning the existing to_keep without any changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, to_keep.extend will keep what's in to_keep, but add from updated, and since we've already removed all toggles from to_keep that matches one of the projects this token has access to, we won't have to worry about duplicates.

Copy link
Member

@nunogois nunogois left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@sighphyre sighphyre left a comment

Choose a reason for hiding this comment

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

Yeah I think this looks good

@chriswk chriswk merged commit 6d434cf into main May 22, 2024
9 checks passed
@chriswk chriswk deleted the fix/unrelatedTokensShouldNotInterfereWithOtherProjects branch May 22, 2024 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants