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: Launchdarkly importer #2530

Merged
merged 36 commits into from
Oct 9, 2023
Merged

feat: Launchdarkly importer #2530

merged 36 commits into from
Oct 9, 2023

Conversation

dabeeeenster
Copy link
Contributor

@dabeeeenster dabeeeenster commented Jul 27, 2023 •
edited by khvn26
Loading

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

  • I have run pre-commit to check linting
  • 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

Implements #2348

How did you test this code?

Please describe.

Left to do

  • Add an API interface that takes an LD API key and an LD project ID and then runs the importer code
  • Front End UI in front of the API

@vercel
Copy link

vercel bot commented Jul 27, 2023 •
edited
Loading

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

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 9, 2023 11:21am
flagsmith-frontend-preview ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 9, 2023 11:21am
flagsmith-frontend-staging ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 9, 2023 11:21am

@dabeeeenster dabeeeenster marked this pull request as draft July 27, 2023 10:31
@github-actions github-actions bot added the api Issue related to the REST API label Jul 27, 2023
@dabeeeenster dabeeeenster changed the title Feature/2348/launch darkly importer feat/Launchdarkly importer Jul 27, 2023
@dabeeeenster dabeeeenster linked an issue Jul 27, 2023 that may be closed by this pull request
LaunchDarkly importer #2348
Closed
@dabeeeenster dabeeeenster added feature New feature or request front-end Issue related to the React Front End Dashboard labels Jul 27, 2023
@dabeeeenster dabeeeenster changed the title feat/Launchdarkly importer feat: Launchdarkly importer Jul 27, 2023
@dabeeeenster dabeeeenster requested review from a team and novakzaballa July 27, 2023 10:33
@github-actions
Copy link
Contributor

github-actions bot commented Jul 27, 2023 •
edited
Loading

Uffizzi Preview deployment-38024 was deleted.

@github-actions github-actions bot removed the front-end Issue related to the React Front End Dashboard label Jul 27, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jul 27, 2023 •
edited
Loading

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (bb080d2) 95.53% compared to head (ef03dbb) 95.59%.
Report is 28 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2530      +/-   ##
==========================================
+ Coverage   95.53%   95.59%   +0.05%     
==========================================
  Files         994     1009      +15     
  Lines       28072    28833     +761     
==========================================
+ Hits        26818    27562     +744     
- Misses       1254     1271      +17
Files Coverage Δ
api/app/settings/common.py 88.11% <ø> (ø)
api/audit/related_object_type.py 100.00% <100.00%> (ø)
api/integrations/launch_darkly/apps.py 100.00% <100.00%> (ø)
api/integrations/launch_darkly/client.py 100.00% <100.00%> (ø)
api/integrations/launch_darkly/constants.py 100.00% <100.00%> (ø)
...egrations/launch_darkly/migrations/0001_initial.py 100.00% <100.00%> (ø)
...nique_project_ld_project_key_status_result_null.py 100.00% <100.00%> (ø)
api/integrations/launch_darkly/serializers.py 100.00% <100.00%> (ø)
api/integrations/launch_darkly/types.py 100.00% <100.00%> (ø)
api/projects/urls.py 100.00% <100.00%> (ø)
... and 8 more

... and 11 files with indirect coverage changes

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

gagantrivedi
gagantrivedi reviewed Oct 3, 2023
View reviewed changes
api/integrations/launch_darkly/tasks.py

@register_task_handler()
def process_launch_darkly_import_request(import_request_id: int) -> None:
with transaction.atomic():
Copy link
Member

@gagantrivedi gagantrivedi Oct 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I am a bit concerned about this long-running traction: is there any way we can avoid this? or reduce it?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe the question is should we have it at all or should we prefer having partial success imports?

To support partial imports better, we could extend the import request status field to have a partial_success status and error_messages array. Moreover, we could have a per-object status report so users could see which exact entity failed to import.

Copy link
Member

Choose a reason for hiding this comment

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

As discussed internally, keeping the transaction for now. Partial imports TBD in a separate PR.

@khvn26 khvn26 requested a review from gagantrivedi October 3, 2023 18:00
gagantrivedi
gagantrivedi approved these changes Oct 9, 2023
View reviewed changes
Copy link
Member

@gagantrivedi gagantrivedi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Let's create an issue for removing transaction before we merge this

@khvn26
Copy link
Member

khvn26 commented Oct 9, 2023

#2838

@khvn26 khvn26 merged commit 4f7464b into main Oct 9, 2023
@khvn26 khvn26 deleted the feature/2348/launch-darkly-importer branch October 9, 2023 13:39
@dabeeeenster dabeeeenster mentioned this pull request Oct 9, 2023
Merged
@khvn26 khvn26 mentioned this pull request Oct 9, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@khvn26 khvn26 khvn26 left review comments

@matthewelwell matthewelwell matthewelwell left review comments

@ajhelsby ajhelsby ajhelsby left review comments

@gagantrivedi gagantrivedi gagantrivedi approved these changes

@novakzaballa novakzaballa Awaiting requested review from novakzaballa novakzaballa was automatically assigned from Flagsmith/flagsmith

Assignees

@khvn26 khvn26

@ajhelsby ajhelsby

Labels
api Issue related to the REST API docs Documentation updates feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

LaunchDarkly importer
6 participants
@dabeeeenster @codecov-commenter @ajhelsby @khvn26 @matthewelwell @gagantrivedi