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: Heal all identities with blank traits #4908

Merged
merged 10 commits into from
Dec 10, 2024
Merged

feat: Heal all identities with blank traits #4908

merged 10 commits into from
Dec 10, 2024

Conversation

khvn26
Copy link
Member

@khvn26 khvn26 commented Dec 10, 2024 •
edited
Loading

Thanks for submitting a PR! Please check the boxes below:

  • I have added information to docs/ if required so people know about the feature!
  • I have filled in the "Changes" section below?
  • I have filled in the "How did you test this code" section below?
  • I have used a Conventional Commit title for this Pull Request

Changes

Adds a management command that scans through all identities and restores previously deleted blank traits.

Additionally, updates local pre-commit version to play nice with recently updated hooks.

Additionally, adds structlog and logs the command progress with structured events.

How did you test this code?

The code is covered by a new unit test. The test ensures only the fixed identities get written.

@khvn26 khvn26 requested a review from a team as a code owner December 10, 2024 10:21
@khvn26 khvn26 requested review from zachaysan and removed request for a team December 10, 2024 10:21
@vercel Vercel
Copy link

vercel bot commented Dec 10, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
docs ⬜️ Ignored (Inspect) Visit Preview Dec 10, 2024 5:50pm
flagsmith-frontend-preview ⬜️ Ignored (Inspect) Visit Preview Dec 10, 2024 5:50pm
flagsmith-frontend-staging ⬜️ Ignored (Inspect) Visit Preview Dec 10, 2024 5:50pm

@github-actions github-actions bot added the api Issue related to the REST API label Dec 10, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 10, 2024 •
edited
Loading

Docker builds report

Image Build Status Security report
ghcr.io/flagsmith/flagsmith-e2e:pr-4908 Finished ✅ Skipped
ghcr.io/flagsmith/flagsmith-api-test:pr-4908 Finished ✅ Skipped
ghcr.io/flagsmith/flagsmith-api:pr-4908 Finished ✅ Results ✅
ghcr.io/flagsmith/flagsmith:pr-4908 Finished ✅ Results ✅
ghcr.io/flagsmith/flagsmith-private-cloud:pr-4908 Finished ✅ Results ✅

@github-actions github-actions bot added the feature New feature or request label Dec 10, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 10, 2024 •
edited
Loading

Uffizzi Preview deployment-58979 was deleted.

@codecov Codecov
Copy link

codecov bot commented Dec 10, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.39%. Comparing base (f97c56f) to head (f6df9eb).
Report is 3 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #4908   +/-   ##
=======================================
  Coverage   97.38%   97.39%           
=======================================
  Files        1189     1190    +1     
  Lines       41453    41527   +74     
=======================================
+ Hits        40371    40445   +74     
  Misses       1082     1082

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

matthewelwell
matthewelwell approved these changes Dec 10, 2024
View reviewed changes
Copy link
Contributor

@matthewelwell matthewelwell left a comment

Choose a reason for hiding this comment

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

Approved with a couple of minor comments.

We should also remove the unrelated code (from the other PR here)

api/edge_api/management/commands/ensure_identity_traits_blanks.py Outdated Show resolved Hide resolved
api/edge_api/management/commands/ensure_identity_traits_blanks.py Outdated
Comment on lines 25 to 27
self.stdout.write(
f"Fixed identity id={identity_document['identity_uuid']}",
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should add some more log messages like this one to show some indication of progress?

I guess we don't want to write a message for every identity we iterate over, but perhaps we should add a counter, and log a message every 100k or something?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we had standardised stats export like Prometheus, sure.

At this point in time I prefer not to do this.

Besides, I don't expect that many corrupted identities.

Copy link
Contributor

Choose a reason for hiding this comment

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

Besides, I don't expect that many corrupted identities.

This is exactly the reason that I think we should do this. Because it might be the case that, when it's running, it looks like nothing is happening.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, added some additional counting logging.

Copy link
Member Author

Choose a reason for hiding this comment

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

NB: I've decided to re-implement it using structlog.

@khvn26 khvn26 force-pushed the feat/ensure-identity-traits-blanks branch from b2a6137 to b2db8b4 Compare December 10, 2024 13:04
@github-actions github-actions bot added feature New feature or request and removed feature New feature or request labels Dec 10, 2024
@github-actions github-actions bot added feature New feature or request and removed feature New feature or request labels Dec 10, 2024
@github-actions github-actions bot added feature New feature or request and removed feature New feature or request labels Dec 10, 2024
@github-actions github-actions bot added feature New feature or request and removed feature New feature or request labels Dec 10, 2024
@github-actions github-actions bot added feature New feature or request and removed feature New feature or request labels Dec 10, 2024
@github-actions github-actions bot added feature New feature or request and removed feature New feature or request labels Dec 10, 2024
@github-actions github-actions bot added feature New feature or request and removed feature New feature or request labels Dec 10, 2024
zachaysan
zachaysan approved these changes Dec 10, 2024
View reviewed changes
Copy link
Contributor

@zachaysan zachaysan left a comment

Choose a reason for hiding this comment

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

Looks good though you're missing some code coverage for a small section of the command.

@github-actions github-actions bot added feature New feature or request and removed feature New feature or request labels Dec 10, 2024
@github-actions github-actions bot added feature New feature or request and removed feature New feature or request labels Dec 10, 2024
@khvn26 khvn26 merged commit 5405cc5 into main Dec 10, 2024
35 checks passed
@khvn26 khvn26 deleted the feat/ensure-identity-traits-blanks branch December 10, 2024 18:01
@flagsmithdev flagsmithdev mentioned this pull request Dec 10, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewelwell matthewelwell matthewelwell approved these changes

@zachaysan zachaysan zachaysan approved these changes

Assignees
No one assigned
Labels
api Issue related to the REST API feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@khvn26 @zachaysan @matthewelwell