Skip to content

fix(flags): allow no member for launchdarkly flag log#83045

Closed
michellewzhang wants to merge 3 commits into
masterfrom
mz/fix-ld-no-member
Closed

fix(flags): allow no member for launchdarkly flag log#83045
michellewzhang wants to merge 3 commits into
masterfrom
mz/fix-ld-no-member

Conversation

@michellewzhang

@michellewzhang michellewzhang commented Jan 7, 2025

Copy link
Copy Markdown
Contributor

relates to SENTRY-3M7C

@michellewzhang michellewzhang requested a review from a team as a code owner January 7, 2025 21:27
@michellewzhang michellewzhang changed the title fix(flags): allow no member for flag log fix(flags): allow no member for launchdarkly flag log Jan 7, 2025
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Jan 7, 2025
@michellewzhang michellewzhang requested a review from aliu39 January 7, 2025 21:27
Comment thread src/sentry/flags/providers.py Outdated
if result.get("member"):
created_by = result["member"]["email"]
else:
created_by = "unknown"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Remove this and allow the value to be null. Looks great!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

discussed on slack - since the createdBy field is required on the model, we'll need to migrate to allow it to be null. will do in a followup. but for now, we'll set it to the empty string.

see followup ticket for more context: #83050

if result.get("member"):
created_by = result["member"]["email"]
else:
created_by = ""

@JoshFerge JoshFerge Jan 7, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why empty string instead of None?

@michellewzhang michellewzhang Jan 7, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

we're going to do this in a followup; it requires a migration since the createdBy column is non-nullable right now. see this ticket for more context:

#83050

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@michellewzhang If you want you can try writing a migration. Its not too difficult. To go from not null to null.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The command line is sentry django [django command...].

@cmanallen cmanallen Jan 7, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Add null=True to these columns:

created_by = models.CharField(max_length=100)
created_by_type = models.PositiveSmallIntegerField(choices=CREATED_BY_TYPE_TYPES)

And then run the normal migration commands.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Optional of course, I can write one tomorrow.

@michellewzhang michellewzhang deleted the mz/fix-ld-no-member branch January 9, 2025 20:34
@github-actions github-actions Bot locked and limited conversation to collaborators Jan 25, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants