Skip to content

Add timestamps migration #195

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add timestamps migration #195

wants to merge 1 commit into from

Conversation

vtm9
Copy link

@vtm9 vtm9 commented Mar 20, 2025

Hey @tompave, I’ve created the first PR containing just migration timestamps.
I love how Oban handles migrations with versioning:
https://github.com/oban-bg/oban/blob/main/lib/oban/migration.ex
Maybe we can implement a similar migrator here, what do you think?

@tompave
Copy link
Owner

tompave commented Mar 21, 2025

Thank you for the work and the PR!

Can you please test this version of the code with and without the migration applied? That's necessary to write upgrade instructions. Ideally, this should be a non-breaking change if we want to ship it in the 1.x line.

@tompave
Copy link
Owner

tompave commented Mar 21, 2025

It would also be good to modify the current migration so that users can copy just one migration file when installing the package for the first time.

For example, look at:

I love how Oban handles migrations with versioning
Maybe we can implement a similar migrator here, what do you think?

Maybe in v2.0, but I think it should not be in this PR.

@vtm9
Copy link
Author

vtm9 commented Mar 26, 2025

make it non breaking might be hard. I'll try something.
Thank you

@vtm9
Copy link
Author

vtm9 commented Mar 29, 2025

Hey @tompave,
I’ve tried multiple approaches, but I still can’t get a good result.
We can use a compile-time flag to include timestamps in the schema during compilation,
but at some point, we’ll need to introduce a breaking change anyway.

Why not release version 2.0 with the breaking changes? That seems like the simplest path forward.
I can provide a migration guide.

Most likely, existing users will just add a migration with timestamps, and new users will include it during table creation.

@tompave
Copy link
Owner

tompave commented Mar 30, 2025

Yes, I was thinking that adding this to v2.0 would be best. In that case it's ok if it's a breaking change.

Please leave the PR open for now. I'll tag it with the v2.0 label.

@tompave tompave added the v2.0 label Mar 30, 2025
Copy link

@kuznetskikh kuznetskikh left a comment

Choose a reason for hiding this comment

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

Very straightforward, but very long awaited changes. Thanks Vitaly!

@tompave
Copy link
Owner

tompave commented Apr 2, 2025

Just to make it easier to resume work for this later:

@vtm9
Copy link
Author

vtm9 commented Apr 2, 2025

hey @tompave , are there any blockers to release v 2.0?
I also want to add timestamps to gate and flag, to show it in the FWF_UI, what do you think?

defmodule FunWithFlags.Flag do

  defstruct name: nil, gates: [], inserted_at: nil, updated_at: nil

defmodule FunWithFlags.Gate do


  defstruct [:type, :for, :enabled, :inserted_at, :updated_at]

  defmodule FunWithFlags.Store.Serializer.Ecto do
    @moduledoc false

    alias FunWithFlags.Flag
    alias FunWithFlags.Gate
    alias FunWithFlags.Store.Persistent.Ecto.Record

    @spec deserialize_flag(atom() | String.t(), list()) :: Flag.t()
    def deserialize_flag(name, []), do: Flag.new(to_atom(name), [])

    def deserialize_flag(name, list) when is_list(list) do
      gates =
        list
        |> Enum.sort_by(& &1.gate_type)
        |> Enum.map(&deserialize_gate(to_string(name), &1))
        |> Enum.reject(&(!&1))

      inserted_at = gates |> Enum.map(& &1.inserted_at) |> Enum.min()
      updated_at = gates |> Enum.map(& &1.updated_at) |> Enum.max()

      Flag.new(to_atom(name), gates, inserted_at, updated_at)
    end

@vtm9
Copy link
Author

vtm9 commented Apr 8, 2025

hey @tompave , sorry for disturbing. I really want to merge timestamps 😄

@tompave
Copy link
Owner

tompave commented Apr 10, 2025

Hey, because of personal reasons it's unlikely I'll be able to pay much attention to this in the short term. If timestamps are critical for you please use your fork. If you want, you can work against the v2 branch to make integration easier later.

@vtm9
Copy link
Author

vtm9 commented Apr 10, 2025

Hey!
I completely understand that personal matters must come first. If there’s anything I can do to help test or collaborate on the v2 branch so that integration is smoother down the road, please let me know.
Wishing you all the best, and thanks again for your time and support!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants