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.

Sign up for GitHub

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

Jump to bottom

migrations: add ReplaceStringMigration migration helper, simplify containerd-config-path migration, host-containers-version migration #712

Merged
merged 3 commits into from
Feb 7, 2020
Merged

migrations: add ReplaceStringMigration migration helper, simplify containerd-config-path migration, host-containers-version migration #712

merged 3 commits into from
Feb 7, 2020

Conversation

etungsten
Copy link
Contributor

@etungsten etungsten commented Feb 4, 2020 •
edited

Issue #, if available: N/A

Description of changes:
Adds new common migration helper ReplaceStringMigration for replacing the string value of some specified setting in the datastore.

Simplifies migrate-contrainerd-config-path with ReplaceStringMigration.

Testing done:
Locally testing updated migrate-containerd-config-path migration, backward migration:

 $ cat /tmp/data-store/current/live/configuration-files/containerd-config-toml/template-path 
"/usr/share/templates/containerd-config-toml_aws-k8s"
$ VARIANT=aws-k8s cargo run -- --datastore-path /tmp/data-store/current --backward
Changed value of 'configuration-files.containerd-config-toml.template-path' from '/usr/share/templates/containerd-config-toml_aws-k8s' to '/usr/share/templates/containerd-config-toml' on downgrade
$ cat /tmp/data-store/current/live/configuration-files/containerd-config-toml/template-path "/usr/share/templates/containerd-config-toml"

Forward migration:

 $ VARIANT=aws-k8s cargo run -- --datastore-path /tmp/data-store/current --forward
Changed value of 'configuration-files.containerd-config-toml.template-path' from '/usr/share/templates/containerd-config-toml' to '/usr/share/templates/containerd-config-toml_aws-k8s' on upgrade

Locally testing updated migrate-host-containers-version migration, backward migration:

✦ $ VARIANT=aws-k8s RUST_BACKTRACE=full cargo run -- --datastore-path /tmp/data-store/current --backward
Changed value of 'settings.host-containers.admin.source' from '328549459982.dkr.ecr.us-west-2.amazonaws.com/thar-admin:v0.2' to '328549459982.dkr.ecr.us-west-2.amazonaws.com/thar-admin:v0.1' on downgrade
Found no 'settings.host-containers.admin.source' to change on upgrade
Changed value of 'settings.host-containers.control.source' from '328549459982.dkr.ecr.us-west-2.amazonaws.com/thar-control:v0.2' to '328549459982.dkr.ecr.us-west-2.amazonaws.com/thar-control:v0.1' on downgrade
Found no 'settings.host-containers.control.source' to change on upgrade

Forward migration:

✦ $ VARIANT=aws-k8s RUST_BACKTRACE=full cargo run -- --datastore-path /tmp/data-store/current --forward
Changed value of 'settings.host-containers.admin.source' from '328549459982.dkr.ecr.us-west-2.amazonaws.com/thar-admin:v0.1' to '328549459982.dkr.ecr.us-west-2.amazonaws.com/thar-admin:v0.2' on upgrade
Found no 'settings.host-containers.admin.source' to change on upgrade
Changed value of 'settings.host-containers.control.source' from '328549459982.dkr.ecr.us-west-2.amazonaws.com/thar-control:v0.1' to '328549459982.dkr.ecr.us-west-2.amazonaws.com/thar-control:v0.2' on upgrade
Found no 'settings.host-containers.control.source' to change on upgrade

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

tjkirch
tjkirch reviewed Feb 4, 2020
View reviewed changes
Copy link
Contributor

@tjkirch tjkirch left a comment •
edited

Choose a reason for hiding this comment

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

Would you please describe the testing you did in the description? It might be easiest to include the change to the existing host-containers-version migration in this PR so you can use that clear example.

workspaces/api/migration/migration-helpers/src/common_migrations.rs Outdated Show resolved Hide resolved
workspaces/api/migration/migration-helpers/src/common_migrations.rs Outdated Show resolved Hide resolved
workspaces/api/migration/migration-helpers/src/common_migrations.rs Outdated Show resolved Hide resolved
workspaces/api/migration/migration-helpers/src/common_migrations.rs Outdated Show resolved Hide resolved
workspaces/api/migration/migration-helpers/src/common_migrations.rs Outdated Show resolved Hide resolved
workspaces/api/migration/migration-helpers/src/common_migrations.rs Outdated Show resolved Hide resolved
@etungsten etungsten force-pushed the replace-setting-migration-helper branch from 6ccc58f to 117897e Compare February 4, 2020 23:23
@etungsten etungsten changed the title migration-helpers: add ReplaceSettingMigration migrations: add ReplaceStringMigration migration helper, simply containerd-config-path migration Feb 4, 2020
@etungsten etungsten changed the title migrations: add ReplaceStringMigration migration helper, simply containerd-config-path migration migrations: add ReplaceStringMigration migration helper, simplify containerd-config-path migration Feb 4, 2020
@etungsten etungsten force-pushed the replace-setting-migration-helper branch from 117897e to f2f1546 Compare February 4, 2020 23:25
@etungsten
Copy link
Contributor Author

etungsten commented Feb 4, 2020 •
edited

Pushes above addresses @tjkirch 's comments.
Adds commit to showcase using ReplaceStringMigration migration helper

Added a testing section to the PR description.

@etungsten etungsten marked this pull request as ready for review February 4, 2020 23:34
tjkirch
tjkirch reviewed Feb 4, 2020
View reviewed changes
Copy link
Contributor

@tjkirch tjkirch left a comment

Choose a reason for hiding this comment

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

Would you please also update the host-container-version migration?

workspaces/api/migration/migrations/v0.2/migrate-containerd-config-path/src/main.rs Outdated Show resolved Hide resolved
workspaces/api/migration/migration-helpers/src/common_migrations.rs Outdated Show resolved Hide resolved
workspaces/api/migration/migration-helpers/src/common_migrations.rs Outdated Show resolved Hide resolved
@etungsten etungsten force-pushed the replace-setting-migration-helper branch 3 times, most recently from a8e20c4 to 71299ab Compare February 5, 2020 00:55
@etungsten etungsten changed the title migrations: add ReplaceStringMigration migration helper, simplify containerd-config-path migration migrations: add ReplaceStringMigration migration helper, simplify containerd-config-path migration, host-containers-version migration Feb 5, 2020
@etungsten
Copy link
Contributor Author

etungsten commented Feb 5, 2020 •
edited

Addresses @tjkirch 's comments.
Second push fixes the commit message.
Third push has minor ordering fixes

Added more testing to the PR description

@etungsten etungsten force-pushed the replace-setting-migration-helper branch from 71299ab to 5573b59 Compare February 5, 2020 18:15
@etungsten
Copy link
Contributor Author

Addresses @tjkirch 's comments.

@etungsten
migration-helpers: add ReplaceStringMigration
5fa25ce 
Adds new common migration `ReplaceStringMigration` for replacing the
string value of some specified setting in the datastore.
@etungsten
migrations: simplify containerd config path migration
cb6f634 
Simplifies containerd config path migration with new
`ReplaceStringMigration` migration helper
@etungsten
migrations: simplify host-containers-version migration with helpers
717cefd 
Use new `ReplaceStringMigration` helper to simplify
host-containers-version migration
@etungsten etungsten force-pushed the replace-setting-migration-helper branch from 5573b59 to 717cefd Compare February 5, 2020 18:31
@etungsten
Copy link
Contributor Author

Addresses @tjkirch 's comment.

Tested migrations and they still work as expected.

@etungsten etungsten requested a review from zmrow February 5, 2020 20:20
zmrow
zmrow approved these changes Feb 7, 2020
View reviewed changes
Copy link
Contributor

@zmrow zmrow left a comment

Choose a reason for hiding this comment

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

🚦

@etungsten etungsten merged commit 79d0331 into develop Feb 7, 2020
@etungsten etungsten deleted the replace-setting-migration-helper branch February 7, 2020 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tjkirch tjkirch tjkirch approved these changes

@zmrow zmrow zmrow approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@etungsten @tjkirch @zmrow