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

Fixes #27534 - Allow migration of Dynflow data using prod2dev #6962

Merged
merged 4 commits into from
Sep 4, 2019

Conversation

adamruzicka
Copy link
Contributor

This needs the user to run rake dynflow:migrate against the target db before running prod2dev.

@theforeman-bot
Copy link
Member

Issues: #27534

@ezr-ondrej
Copy link
Member

Can't we use pure arel for the data migrations? I believe we would have all the necessary data type perks, but we would avoid ActiveRecord overhead.
Second thing why we are migrating dynflow in core? Can't we extend the task from foreman-tasks?

@adamruzicka
Copy link
Contributor Author

About arel, we could, but I didn't want to rewrite the migration script from scratch.

About migration, by default foreman uses dynflow as a backend for active job. So it is possible to have dynflow and not have foreman-tasks. This means we also need to have the migration in core and not in tasks.

@ezr-ondrej
Copy link
Member

Ack why here, thanks for explanation :)

About arel, we could, but I didn't want to rewrite the migration script from scratch.

Just it feels like it would have been less of a hassle... but I agree, if it works, do not touch it unless necessary.

Another question though: If we do not have the dynflow, what is the rake task doing than? won't it create the dynflow tables anyways?

@adamruzicka
Copy link
Contributor Author

If we do not have the dynflow

What do you mean by this? We should always have dynflow tables on the source side. If not they will get created on the destination side, but will remain empty.

@ezr-ondrej
Copy link
Member

What do you mean by this? We should always have dynflow tables on the source side. If not they will get created on the destination side, but will remain empty.

Ah, I thought I can have foreman without dynflow, but it's always around. Sorry for the fuzz, going to test then 👍

# dynflow:migrate migrates the db configured for the current rails env
# In this case, we need to make sure it migrates development
env_bak = ::Rails.env
Rake::Task['dynflow:migrate'].invoke
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this just run with development env? since we are always migrating production=>development.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, we are always migrating production => development, regardless of what ::Rails.env is set to. dynflow:migrate is picky about this and always migrates the db associated with the current rails environment.

Copy link
Member

Choose a reason for hiding this comment

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

can we move this to the mysql2pgsql task @lzap added? Or maybe it should be done as part of the db:migrate task?

# In this case, we need to make sure it migrates development
env_bak = ::Rails.env
Rake::Task['dynflow:migrate'].invoke
::Rails.env = env_bak
Copy link
Member

Choose a reason for hiding this comment

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

why do we need to do this? does the dynflow task modify the environment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to change the current environment to always migrate the db associated with development, regardless of what ::Rails.env is set to. It felt right to restore the previous value when we don't need it anymore

Copy link
Member

Choose a reason for hiding this comment

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

where do we change that? iiuc this runs before the migration script

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tbrisker tbrisker added this to the 1.23.0 milestone Aug 26, 2019
tbrisker
tbrisker previously approved these changes Sep 3, 2019
Copy link
Member

@tbrisker tbrisker left a comment

Choose a reason for hiding this comment

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

Thanks @adamruzicka ! this needs a rebase though (but perhaps wait with that until GH-6974 is in)

@tbrisker
Copy link
Member

tbrisker commented Sep 3, 2019

Please rebase now that the other PR was merged.

@adamruzicka
Copy link
Contributor Author

Rebased

Copy link
Member

@tbrisker tbrisker left a comment

Choose a reason for hiding this comment

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

Thanks @adamruzicka !

@tbrisker tbrisker merged commit 6e32026 into theforeman:develop Sep 4, 2019
@tbrisker
Copy link
Member

tbrisker commented Sep 4, 2019

1.23 - 64420e7 2db66cc f29ec63 23a9de3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants