Update Flipper #4489
Update Flipper #4489
Conversation
Gemfile
Outdated
| gem 'flipper-ui', '~> 0.25.0' | ||
| gem 'flipper' | ||
| gem 'flipper-active_record' | ||
| gem 'flipper-ui' |
There was a problem hiding this comment.
Would it be preferable to lock the versions again to the newly identified ones during dependency resolution? i.e. 1.3.1 for each of these.
I know this can cause dependency lockout in the future, but it stops these randomly upgrading in the future and causing issues because of a regression or because of breaking changes in a version update.
There was a problem hiding this comment.
I'm inclined to leave it unlocked, but can see the value of preventing breaking upgrades from happening automatically. I can lock it to the major version now that we are in 1.x land?
There was a problem hiding this comment.
Locking to the major version would make me more comfortable than leaving it free to upgrade itself whenever it feels like it.
| class ChangeFlipperGatesValueToText < ActiveRecord::Migration[6.1] | ||
| def up | ||
| # Ensure this incremental update migration is idempotent | ||
| return unless connection.column_exists? :flipper_gates, :value, :string |
There was a problem hiding this comment.
Is this really needed? What would happen if the column were not already containing a string value, other than spending a bit longer than strictly needed doing the migration? If you do think it's needed, should it also be used in the down method?
There was a problem hiding this comment.
Both this and the comment in the down method are very good points.
In this case the migration was automatically created by running bundle exec rails g flipper:active_record --force as requested by the upgraded version of the package, so I cannot take credit for this code.
But since it is making changes to our codebase I think you're right that it should meet some quality standards - such as being truely reversable.
Since I haven't suceeded in getting the cucumber tests to pass due to the migration - I'll give it a good once over.
There was a problem hiding this comment.
Another alternative is to mark it as non-reversible as we've done in some previous migrations (I think I've seen it here anyway)
| end | ||
|
|
||
| def down | ||
| change_column :flipper_gates, :value, :string |
There was a problem hiding this comment.
Does this also need to do the index modifications to be a true reversal of the above?
Co-authored-by: Stuart McHattie <[email protected]>
Prevents breaking upgrades
|
I can't get the cucumber tests to pass, putting on hold for now. |
StephenHulme
added
depfu
Flipper was causing dependency lock with Sinatra (see Depfu).
Changes proposed in this pull request
Instructions for Reviewers
[All PRs] - Confirm PR template filled
[Feature Branches] - Review code
[Production Merges to
main]- Check story numbers included
- Check for debug code
- Check version