feat: Create flagsmith on flagsmith feature export task #3149
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
zachaysan
changed the title
|
Uffizzi Preview |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3149 +/- ##
==========================================
- Coverage 96.11% 96.10% -0.01%
==========================================
Files 1058 1059 +1
Lines 31918 31995 +77
==========================================
+ Hits 30677 30750 +73
- Misses 1241 1245 +4 ☔ View full report in Codecov by Sentry. |
api/features/import_export/tasks.py
Outdated
| @register_recurring_task( | ||
| run_every=timedelta(hours=24), | ||
| ) | ||
| def create_flagsmith_on_flagsmith_feature_export(): | ||
| if ( | ||
| not settings.FLAGSMITH_ON_FLAGSMITH_FEATURE_EXPORT_ENVIRONMENT_ID | ||
| or not settings.FLAGSMITH_ON_FLAGSMITH_FEATURE_EXPORT_TAG_ID | ||
| ): | ||
| # No explicit settings on both feature settings, hence exit. |
There was a problem hiding this comment.
I'd suggest that probably the settings filter should be before we register the recurring task, otherwise it might be a little odd for self hosted people that see the task registered.
e.g.
if (settings.FLAGSMITH_ON_FLAGSMITH_FEATURE_EXPORT_ENVIRONMENT_ID and settings.FLAGSMITH_ON_FLAGSMITH_FEATURE_EXPORT_TAG_ID):
@register_recurring_task(
run_every=timedelta(hours=24),
)
def create_flagsmith_on_flagsmith_feature_export():
...There was a problem hiding this comment.
As we chatted briefly on Slack, there is the issue of importing this so in the mean time I've wrapped a private function and used it in conjunction with your suggestion to keep the task from queuing for the other clients.
api/features/import_export/models.py
Outdated
| class FlagsmithOnFlagsmithFeatureExport(models.Model): | ||
| feature_export = models.ForeignKey( | ||
| FeatureExport, | ||
| related_name="flagsmith_on_flagsmith", | ||
| on_delete=models.CASCADE, | ||
| ) | ||
|
|
||
| created_at = models.DateTimeField(auto_now_add=True) |
There was a problem hiding this comment.
Interesting. I can certainly see why you went with this approach but I think I'd prefer to just have an attribute on the FeatureExport class. Just thinking it's slightly better aesthetics for the self hosted community where this functionality will be totally redundant. It'll be a redundant column vs a redundant table.
That being said, I don't have a massive preference here so if you think this is the cleanest approach, I'll take your word for it.
There was a problem hiding this comment.
I think it's better to keep the whole functionality limited to it's own class. I can see FeatureExport getting expanded and used more and more as the functionality evolves and every dev investigating it will see a field that is used almost none of the time and it will clutter up the definition of the FeatureExport class. I think it's better to just have a single class here which self-describes itself. Plus, if there are any other features we decide to implement in the future, like say hold onto different versions of flagsmith for download, we can add it to the FlagsmithOnFlagsmithFeatureExport class.
There was a problem hiding this comment.
Yep, fair enough. Happy with this approach in that case. Maybe we should add a docstring to the model however, to help explain it's purpose.
There was a problem hiding this comment.
Good idea I'll add it before merging the PR.
zachaysan
deleted the
feat/create_flagsmith_on_flagsmith_feature_export_task
branch
Thanks for submitting a PR! Please check the boxes below:
pre-committo check lintingdocs/if required so people know about the feature!Changes
This enables the easy export of Flagsmith feature flag features through the existing feature import export module. By setting two new settings constants, one for the target environment id and one for the tag id that will be selected for the feature flags that are to be exported, a daily background task will create new export ids which are saved through a new model. The most recent of which will be served through the download endpoint which is fixed and open to all without authentication.
How did you test this code?
Three new tests. The actual data itself is assert in both the task generation test and the view test.