Skip to content

Conversation

@rjsheperd
Copy link
Contributor


Purpose

EOM

@Kcheung42
Copy link
Collaborator

The update to using the git hash to store in a migration entity looks good.

One thing I noticed however in already committed code

help_import.clj

ln 97: Has sm/rollback-tx! been working for you? It seems like it shouldn't since sm/rollback-tx! expects a transaction id or transaction result map from datomic.api/transact. Instead, here a datomic entity id is being passed (result from )q-tx. I wonder if along with the :bp/migration-id, this entity should also store transaction id or ids (in case a migration needs a multi transaction process). These transaction ids would need to be sorted too so we know how to reverse them when rolling back.

(defn rollback-import
  "Rollback imports. Option to provide a transaction date."
  [& args]
  (cms/init-db!)
  (let [a-datetime   (first args)
        conn         @ds/datomic-conn
        db           (d/db conn)
        -now         (now)
        tx-remove-id (q-tx db (format "help-import-remove-%s" (or a-datetime -now)))
        tx-add-id    (q-tx db (format "help-import-add-%s" (or a-datetime -now)))]

    (sm/rollback-tx! conn tx-remove-id)
    (sm/rollback-tx! conn tx-add-id)))

@rjsheperd
Copy link
Contributor Author

Thanks for catching this, Kenny.

I've gone ahead and updated the q-tx function to return the tx of
the migration Datom.

Regarding this comment:

I wonder if along with the :bp/migration-id, this entity should also store transaction id or ids (in case a migration needs a multi transaction process). These transaction ids would need to be sorted too so we know how to reverse them when rolling back.

The transaction ID from the Datom is stored as the fourth element, ([E A V TX OP]) is already stored in the migration Datom. Not sure what
you mean a "migration that needs a multi-transaction process"?

This has made me think that we could wrap the help-import-add-### and help-import-remove-#### tx's into one transaction (e.g. help-import-####) so that the rollback would be just the one tx.

@Kcheung42
Copy link
Collaborator

Kcheung42 commented Apr 9, 2025

I'm not seeing where the transaction is being stored in the migration datom. Looking at ln 86-94 block:

(when-not existing-tx?
      #_(println (format "Removing old help docs from hash: %s" -now))
      (d/transact conn (concat [(sm/->migration remove-tx)]
                               (remove-existing-pages-tx db en-us)))

      #_(println (format "Adding new help docs %s" -now))
      (d/transact conn (concat [(sm/->migration add-tx)]
                               (cleaned-topics-tx behave-docs en-us)
                               (cleaned-variables-tx behave-docs en-us))))

The transaction from d/transact isn't being stored. A migration entity is being created via sm/->migration but this entity doesn't have the transaction info. In the rollback-import! it's only deleting the migration entity but not actually rolling back the migration itself.

What I think we'd need is something like this.

(let [tx1    @(d/transact conn (remove-existing-pages-tx db en-us))
      tx2    @(d/transact conn (concat (cleaned-topics-tx behave-docs en-us)
                                       (cleaned-variables-tx behave-docs en-us)))
      tx-id1 (nth (first (:tx-data tx1)) 3)
      tx-id2 (nth (first (:tx-data tx2)) 3)]
  (d/transact conn (sm/->migration {:id remove-tx ;NOTE sm/->migration and the schema would need to be updated accordingly
                                    :txs [tx-id1 tx-id2]})))

@Kcheung42
Copy link
Collaborator

Kcheung42 commented Apr 9, 2025

Regarding the multi transaction migration. Take a look at migrations.2025-03-13-rearrange-group-variables-crown-size-submodule

This migration needed to move a set of group variables from one group to another. I had to break this up into two separate transactions:

  1. Move groups from one to another and delete the ref in the old group
  2. Delete the old groups

I had to do this because group-variables in the schema is group-variables are components in groups. Deleting an entity will delete it's components too. I guess we have this to facilitate d/touch and we didn't expect group variables to move around.

{:db/ident       :group/group-variables
 :db/doc         "Group's group variables."
 :db/valueType   :db.type/ref
 :db/cardinality :db.cardinality/many
 :db/isComponent true}

@rjsheperd
Copy link
Contributor Author

Good point. I wonder if we can do something where each sub-migration must be named separately? Maybe we can use a :meta tag on the namespace to list the sub-migrations?

Ideally we want to get to a place where each migration namespace (or sub-migration) provides the migration identifier, and can be easily identified.

@Kcheung42
Copy link
Collaborator

Could the migration identifier be the namespace of the migration script?

@rjsheperd
Copy link
Contributor Author

Totally. I had a script that I had written for this. Let me add that to this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants