Skip to content
This repository has been archived by the owner on Aug 3, 2023. It is now read-only.

Tagged Durable Objects Migrations #1992

Merged
merged 3 commits into from Sep 10, 2021
Merged

Conversation

xortive
Copy link
Contributor

@xortive xortive commented Jul 12, 2021

fixes #1950

This PR adds support for tagged migrations to wrangler. Migration tags are a way to ensure that you are applying the correct migration to a given script that contains a DO.

They are also used for allowing multiple migrations to be specified in a list in wrangler.toml -- wrangler will fetch the script's current migration tag and only send migrations after it in the list.

Migration tags can be used in two ways:

  • Specifying --old-tag and --new-tag on wrangler publish.
    --old-tag can only be omitted if the script doesn't have a migration tag already, otherwise it must be provided (so it can be verified against the script's actual current migration tag)

  • The [[migrations]] list in wrangler.toml

[durable_objects]
bindings = [{name = "COUNTER", class_name = "Counter"}]

[[migrations]]
tag = "v1"
new_classes = ["Counter"]

With [[migrations]] you can just wrangler publish without having to use --new-class on the first upload, wrangler will look at the current migration tag of the script and handle everything for you.

testing instructions:

  • clone the malonso/tagged-migrations branch
  • in the root of the repository run cargo install --path . --debug.
  • add your migrations to wrangler.toml

full syntax for [[migrations]]

[[migrations]]
tag = "v1" # should be unique
new_classes = ["Counter"] # array of new classes
deleted_classes = ["DeprecatedClass"] # array of deleted classes
renamed_classes = [{from: "CounterOld", to: "CounterNew"}] # array of rename directives
transferred_classes = [{from_script: "oldCounterScript", from: "Counter", to: "Counter"}] # array of transfer directives


# The migrations list is ordered -- only migrations *after* the script's current migration tag will be run
[[migrations]]
tag = "v2"
# More directives here. You can have multiple types of directives in a single migration, with multiple instances of each directive.

@xortive xortive requested a review from a team as a code owner July 12, 2021 22:26
@xortive xortive requested a review from a-robinson July 12, 2021 22:27
Copy link
Member

@a-robinson a-robinson 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!

target.account_id.load()?
);

let res: ListScriptsV4ApiResponse = client.get(&addr).send()?.json()?;
Copy link
Member

Choose a reason for hiding this comment

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

I know we talked about it, but I'd personally leave a comment here on why we have to list the scripts here rather than something more targeted. It'll help some poor future developer understand why we're doing this and whether it's safe to change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! I'll add one

None => MigrationTag::NoScript,
};

log::info!("Current MigrationTag: {:#?}", tag);
Copy link
Member

Choose a reason for hiding this comment

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

Was this just here for debugging purposes or is the intent to leave it in? I'm fine either way, just calling it out in case it wasn't intentional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

these only show up with RUST_LOG set -- it's nice to have around for issues that get filed

provided_old_tag: Some(provided_tag),
..
} if provided_tag != script_tag => anyhow::bail!(
"The migration tag provided with your adhoc migration (\"{}\") does not match the script's current migration tag of \"{}\", \
Copy link
Member

Choose a reason for hiding this comment

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

nit, but do users know what "adhoc" means in this context? I'd leave that word out.

Or if we leave it in, "ad hoc" is the much more common way of spelling it :)

.map_or_else(Vec::new, |m| vec![m.clone()]),
};

log::info!("Api Migration: {:#?}", api_migration);
Copy link
Member

Choose a reason for hiding this comment

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

Same question about whether this was intentional. If it was, I'd capitalize API.

Ditto below in the ::List case as well.

@nilslice
Copy link
Contributor

Let's hold on merging this - @Electroid should review, and at least one dev prod eng team member. We're planning a release for tomorrow, and unsure if this should be included or not.

@kristianfreeman
Copy link
Contributor

can we make sure to start work on a docs PR when this goes out? thanks!!

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

Successfully merging this pull request may close these issues.

wrangler publish --rename-class and --transfer-class arguments help is insufficient
6 participants