-
Notifications
You must be signed in to change notification settings - Fork 337
Conversation
…ration to structopt
80982d7
to
251ae5b
Compare
There was a problem hiding this 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()?; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
src/settings/toml/migrations.rs
Outdated
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 \"{}\", \ |
There was a problem hiding this comment.
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 :)
src/settings/toml/migrations.rs
Outdated
.map_or_else(Vec::new, |m| vec![m.clone()]), | ||
}; | ||
|
||
log::info!("Api Migration: {:#?}", api_migration); |
There was a problem hiding this comment.
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.
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. |
51eb7d4
to
9c55dd3
Compare
can we make sure to start work on a docs PR when this goes out? thanks!! |
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
onwrangler 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.tomlWith
[[migrations]]
you can justwrangler 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:
malonso/tagged-migrations
branchcargo install --path . --debug
.wrangler.toml
full syntax for
[[migrations]]