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: update consumers when custom_id is also set #707

Merged
merged 1 commit into from
Oct 25, 2022

Conversation

GGabriele
Copy link
Collaborator

Right now decK only uses ID and Username for Consumers lookup,
which is problematic because CustomID is also unique and
modifying a Username causes a Consumer re-creation, which
triggers a collision for the CustomID field.

This PR makes sure that CustomID is included in the lookup
loop, so that such Consumers changes can be made in-place.

Solves #698

@GGabriele GGabriele requested a review from a team June 29, 2022 09:26
@GGabriele GGabriele force-pushed the fix/update-consumer-with-custom-id branch from cd0b014 to c27845a Compare June 29, 2022 09:26
@codecov-commenter
Copy link

codecov-commenter commented Jun 29, 2022

Codecov Report

Merging #707 (c27845a) into main (5df93b6) will decrease coverage by 0.02%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##             main     #707      +/-   ##
==========================================
- Coverage   43.50%   43.47%   -0.03%     
==========================================
  Files          74       74              
  Lines        8923     8933      +10     
==========================================
+ Hits         3882     3884       +2     
- Misses       4671     4677       +6     
- Partials      370      372       +2     
Impacted Files Coverage Δ
state/consumer.go 80.00% <25.00%> (-2.36%) ⬇️
file/builder.go 54.28% <37.50%> (-0.25%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5df93b6...c27845a. Read the comment docs.

@GGabriele GGabriele force-pushed the fix/update-consumer-with-custom-id branch from c27845a to 57a5e97 Compare July 8, 2022 15:06
@GGabriele GGabriele force-pushed the fix/update-consumer-with-custom-id branch from 57a5e97 to 5a288f7 Compare October 5, 2022 17:25
@GGabriele GGabriele temporarily deployed to Configure ci October 5, 2022 17:26 Inactive
@GGabriele GGabriele temporarily deployed to Configure ci October 5, 2022 17:27 Inactive
Right now decK only uses ID and Username for Consumers lookup,
which is problematic because CustomID is also unique and
modifying a Username causes a Consumer re-creation, which
triggers a collision for the CustomID field.

This PR makes sure that CustomID is included in the lookup
loop, so that such Consumers changes can be made in-place.
@GGabriele GGabriele force-pushed the fix/update-consumer-with-custom-id branch from 5a288f7 to 8cdd4a8 Compare October 12, 2022 07:59
@GGabriele GGabriele temporarily deployed to Configure ci October 12, 2022 07:59 Inactive
@GGabriele GGabriele temporarily deployed to Configure ci October 12, 2022 07:59 Inactive
Copy link
Contributor

@aboudreault aboudreault left a comment

Choose a reason for hiding this comment

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

LGTM!

@GGabriele GGabriele merged commit 0ccecff into main Oct 25, 2022
@GGabriele GGabriele deleted the fix/update-consumer-with-custom-id branch October 25, 2022 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants