Skip to content
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

fix bugs, rationalize code, support unix sockets #15

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

Conversation

thwarted
Copy link
Member

  • support unix(/path) for go-style DATABASE_DSN
  • vim is both more ubiquitous than nano and well known/expected to be the default editor
  • remove prev_id in migration plan output, it didn't seem to contribute anything and just made the output confusing
  • make sure the saved snapshot sql file ends in at least one newline, as is the POSIX "text file" way
  • put a blank line between each table definition in the saved snapshot
  • downgrade crashed because it was looking for the no-dump option which wasn't defined for downgrade
  • don't try to "figure out" if the operator specified upgrade or downgrade, and then get it entirely wrong, be explicit and indicate that to the planner
  • since we're not figuring out if this is an upgrade or downgrade, you can now downgrade to a specific version instead of it trying to upgrade to latest when trying to downgrade (or something, I'm not sure what the hell was happening with this code path, but it didn't do anything sane)
  • refactor MigrationRunner plan into subfunctions to reduce cyclomatic complexity by not branching for upgrade or downgrade at every expression and statement
  • use u32 instead of usize for migration ids: these are not pointers; 4B migrations is more than sufficient; the platform's pointer size shouldn't influence how many migrations this tool supports
  • bump version 0.5.0

- support unix(/path) for go-style DATABASE_DSN
- vim is both more ubiquitous than nano and well known/expected to be the default editor
- remove prev_id in migration plan output, it didn't seem to contribute anything and just made the output confusing
- make sure the saved snapshot sql file ends in at least one newline, as is the POSIX "text file" way
- put a blank line between each table definition in the saved snapshot
- downgrade crashed because it was looking for the no-dump option which wasn't defined for downgrade
- don't try to "figure out" if the operator specified upgrade or downgrade, and then get it entirely wrong, be explicit and indicate that to the planner
- since we're not figuring out if this is an upgrade or downgrade, you can now downgrade to a specific version instead of it trying to *upgrade* to latest when trying to downgrade (or something, I'm not sure what the hell was happening with this code path, but it didn't do anything sane)
- refactor MigrationRunner plan into subfunctions to reduce cyclomatic complexity by not branching for upgrade or downgrade at every expression and statement
- use u32 instead of usize for migration ids: these are not pointers; 4B migrations is more than sufficient; the platform's pointer size shouldn't influence how many migrations this tool supports
- bump version 0.5.0
@@ -103,6 +103,7 @@ static DSN_REGEX: Lazy<Regex> = Lazy::new(|| {
pub(crate) struct GoDatabaseDsn {
username: Option<String>,
password: Option<String>,
protocol: String,
Copy link

Choose a reason for hiding this comment

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

If I read this PR correctly, I think you're just supporting tcp and unix as protocols. It may be worth making this an enum. This would make the line if self.protocol == "unix" be more like match self.protocol { Protocol::Unix => {...}, Protocol::TCP => { ... } } which is cleaner. Looking at the list of mysql protocols, unix and tcp are the ones you'll care about so it's probably not worth it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I considered that, but decided not to for that same reason.

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.

None yet

2 participants