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

Add import append flag to import new flags/segments only not existing in DB yet #3299

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

wtertius
Copy link
Contributor

@wtertius wtertius commented Jul 23, 2024

Closes: #2114

@wtertius wtertius requested a review from a team as a code owner July 23, 2024 08:11
@wtertius wtertius force-pushed the add_append_mode_for_import branch 2 times, most recently from 93318ec to 90452d2 Compare July 23, 2024 08:17
@wtertius
Copy link
Contributor Author

Our developers keep flags and segments as code in flipt.yaml config in import format. So, it's convenient to add new flags to the same file. In some way it becomes a way to roll out migration of new flags and segments only without changing state of already existing flags and segments.

@wtertius wtertius changed the title Add import append flag to import new flags only not existing in DB yet Add import append flag to import new flags/segments only not existing in DB yet Jul 23, 2024
@wtertius wtertius force-pushed the add_append_mode_for_import branch from 90452d2 to a2b583b Compare July 23, 2024 08:26
@erka
Copy link
Collaborator

erka commented Jul 23, 2024

Hey @wtertius.

Thank you for your contribution! Your use case is fascinating, and it would be helpful to understand it better. Since you already have a file containing all the flags, have you considered using a declarative storage? Is there any blocker?

internal/ext/importer_test.go Outdated Show resolved Hide resolved
@wtertius
Copy link
Contributor Author

Since you already have a file containing all the flags, have you considered using a declarative storage? Is there any blocker?

Flipt is deployed to Kubernetes namespace with the service itself and its Posgres DB. We chose to use Postgres as Flipt storage basically because we have it already. But also there are other reasons:

  1. Service owner can change flags in UI in realtime at prod environment and we won't loose data as we have automated backups
  2. This allows us to keep more load using Postgres RO replicas for flag evaluating in the future (there is can be feature request for special DSN fo RO to not setup two Flipt instances - one for master and another one for evaluating)
  3. This allows us to make seamless flipt rollout in Kubernetes (that achievable with declarative storage with shared volumes of cause)

Copy link

codecov bot commented Jul 23, 2024

Codecov Report

Attention: Patch coverage is 71.42857% with 16 lines in your changes missing coverage. Please review.

Project coverage is 64.22%. Comparing base (8795205) to head (116d6e4).

Files Patch % Lines
cmd/flipt/import.go 0.00% 8 Missing ⚠️
internal/ext/importer.go 83.33% 4 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3299      +/-   ##
==========================================
+ Coverage   64.18%   64.22%   +0.03%     
==========================================
  Files         168      168              
  Lines       13506    13559      +53     
==========================================
+ Hits         8669     8708      +39     
- Misses       4174     4184      +10     
- Partials      663      667       +4     
Flag Coverage Δ
unittests 64.22% <71.42%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@wtertius wtertius force-pushed the add_append_mode_for_import branch from a2b583b to 396a49d Compare July 23, 2024 10:30
@wtertius wtertius force-pushed the add_append_mode_for_import branch 2 times, most recently from 5f817f2 to 817d611 Compare July 23, 2024 15:58
@markphelps
Copy link
Collaborator

Thank you for your contribution @wtertius !! It makes a lot of sense to me.

Correct me if Im wrong but this PR seems like it's very close to whats being asked for in #2114 ?

What do you think about instead changing this to an upsert operation? So that it adds any new flags/segments but also updates any that have changed?

I guess that would also require us to go through each of the subrecords too (like variants, rules, rollouts, constraints, etc) and check to see if they already exist as well and if so we do an update, and if not we do an create

WDYT @erka ?

@erka
Copy link
Collaborator

erka commented Jul 23, 2024

Correct me if Im wrong but this PR seems like it's very close to whats being asked for in #2114 ?

I think this will resolve that issue.

WDYT @erka ?

I am not sure about upsert. It looks like the database is the state of truth for a specific environment. This means product teams can directly modify flag rules and segments through the UI. While developers appreciate an easy way to introduce new flags/segments. I see it as mixin of declarative and relational storages or better to say a continuous deployment of Flipt for relational databases. Upsert the flags from file could revert the changes introduced by product teams and impact evaluation results.

Copy link
Collaborator

@markphelps markphelps left a comment

Choose a reason for hiding this comment

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

nit: i think maybe we should rename the internal value from newFlagsOnly to append or insertOnly like you have for the external facing command and update the description of the flag

cmd/flipt/import.go Outdated Show resolved Hide resolved
cmd/flipt/import.go Outdated Show resolved Hide resolved
@markphelps
Copy link
Collaborator

Correct me if Im wrong but this PR seems like it's very close to whats being asked for in #2114 ?

I think this will resolve that issue.

WDYT @erka ?

I am not sure about upsert. It looks like the database is the state of truth for a specific environment. This means product teams can directly modify flag rules and segments through the UI. While developers appreciate an easy way to introduce new flags/segments. I see it as mixin of declarative and relational storages or better to say a continuous deployment of Flipt for relational databases. Upsert the flags from file could revert the changes introduced by product teams and impact evaluation results.

yeah I think this will complete part of #2114, it wont fix the 'update' part requested in this comment:

causes the import process to update any existing flags/segments, and add new ones.

However it will fix the overall issue of not having the import fail for existing data. Im good with keeping this change as is then, just want to make sure we come up with the best name possible for the user facing CLI option and description

cmd/flipt/import.go Outdated Show resolved Hide resolved
@wtertius wtertius force-pushed the add_append_mode_for_import branch 4 times, most recently from 204519b to 116d6e4 Compare July 24, 2024 08:52
@wtertius wtertius requested a review from markphelps July 24, 2024 08:53
Copy link
Collaborator

@markphelps markphelps left a comment

Choose a reason for hiding this comment

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

looks great @wtertius ! one minor suggestion about err messages

internal/ext/importer.go Outdated Show resolved Hide resolved
@markphelps
Copy link
Collaborator

@all-contributors please add @wtertius for code

Copy link
Contributor

@markphelps

I've put up a pull request to add @wtertius! 🎉

Copy link
Collaborator

@markphelps markphelps left a comment

Choose a reason for hiding this comment

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

thanks @wtertius !!!

@markphelps markphelps added the automerge Used by Kodiak bot to automerge PRs label Jul 24, 2024
@wtertius wtertius force-pushed the add_append_mode_for_import branch from 027b769 to ad38bcd Compare July 24, 2024 13:35
…sting in DB yet

Signed-off-by: WTertius <wtertius@gmail.com>
@wtertius wtertius force-pushed the add_append_mode_for_import branch from ad38bcd to 3881ba8 Compare July 24, 2024 13:37
@markphelps markphelps added the needs docs Requires documentation updates label Jul 24, 2024
@kodiakhq kodiakhq bot merged commit dae029c into flipt-io:main Jul 24, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Used by Kodiak bot to automerge PRs needs docs Requires documentation updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FLI-666] Add a new import flag to continue the import when an existing item is found
4 participants