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

Install migrator and migration helpers #125

Merged
merged 1 commit into from
Aug 12, 2019
Merged

Install migrator and migration helpers #125

merged 1 commit into from
Aug 12, 2019

Conversation

sam-aws
Copy link
Contributor

@sam-aws sam-aws commented Aug 1, 2019

Issue #, if available:
#90

Description of changes:
Install the migrator tool and individual migration helpers into the
image. This defines a 'migration' package that must be required-by the
release package, and extends the cargo_install macro to optionally
specify the root directory.
Migration versions are only specified in the install for-loop at the
moment, but otherwise are automatically handled by populating a
migration-binaries file to use with %files -f

Signed-off-by: Samuel Mendoza-Jonas samjonas@amazon.com

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

@sam-aws
Copy link
Contributor Author

sam-aws commented Aug 1, 2019

This mostly addresses #90, with some points worth discussing:

  • Defining which migrations to install
    Right now this is just a list of versions defined in the %install step which are then iterated over, but depending on how often this is updated there may be a more user friendly location to define it.

  • All migrations are built unconditionally.
    Because the api %build step is just %cargo_build --all all migrations are built regardless if they will be installed. Unless the number of migrations gets truly out of hand this is probably fine but worth pointing out.

  • Migration workspace & naming
    At the moment there's only the v1.1/testmigration migration as an example, but i
    t sounds like we probably expect a list of potential migration projects like:

v1.1/foo
v1.1/bar
v1.2/foo

So we'd also need to decide on a list of migration names per version, unless we unconditionally install every migration for a given version.

@tjkirch
Copy link
Contributor

tjkirch commented Aug 1, 2019

we'd also need to decide on a list of migration names per version, unless we unconditionally install every migration for a given version.

Assuming we want migrations for version X, we'd want every migration under X. The names are just there for the developer of the migration to briefly explain their purpose. Anything tagged to a specific version is expected to be required for that version.

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.

Nice! What do you think is left before merging? I could probably help test the branch.

packages/api/api.spec Outdated Show resolved Hide resolved
packages/api/api.spec Outdated Show resolved Hide resolved
packages/api/api.spec Outdated Show resolved Hide resolved
packages/api/api.spec Outdated Show resolved Hide resolved
@sam-aws sam-aws marked this pull request as ready for review August 1, 2019 22:43
Copy link
Contributor

@bcressey bcressey left a comment

Choose a reason for hiding this comment

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

LGTM

packages/api/api.spec Outdated Show resolved Hide resolved
packages/api/api.spec Outdated Show resolved Hide resolved
packages/api/api.spec Outdated Show resolved Hide resolved
packages/api/api.spec Outdated Show resolved Hide resolved
macros/cargo Show resolved Hide resolved
packages/api/api.spec Outdated Show resolved Hide resolved
packages/api/api.spec Outdated Show resolved Hide resolved
packages/api/api.spec Outdated Show resolved Hide resolved
packages/api/api.spec Outdated Show resolved Hide resolved
packages/api/api.spec Outdated Show resolved Hide resolved
Install the migrator tool and individual migration helpers into the
image. This defines a 'migration' package that must be required-by the
release package, and extends the cargo_install macro to optionally
specify the root directory.
Migration versions are specified in a macro which is used in the
%install step to install all migrations found for each version.

Signed-off-by: Samuel Mendoza-Jonas <samjonas@amazon.com>
@sam-aws sam-aws merged commit b6c25ee into develop Aug 12, 2019
@sam-aws sam-aws deleted the migrations branch August 12, 2019 17:23
@sam-aws sam-aws mentioned this pull request Oct 2, 2019
4 tasks
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

3 participants