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 migrations being run from a plugin #22626

Merged
merged 1 commit into from
Jul 21, 2023

Conversation

Fryguy
Copy link
Member

@Fryguy Fryguy commented Jul 21, 2023

Before this commit, when running bin/setup on a fresh checkout of
ui-classic, the migrate_database step would fail because it would run in
the spec/manageiq directory as opposed to the plugin root directory.

What happens is that the spec/manageiq directory doesn't have a
Gemfile.lock, because the bundle install happens in the plugin root, and
so when migrate_database runs db:migrate from the spec/manageiq
directory it fails saying it can't find certain gems.

This commit changes it to run db:migrate from the plugin root directory.
Additionally, I've added a create_database step, which is a noop when
the database exists, but also simplifies brand new checkout runs.

@agrare Please review.

Before this commit, when running bin/setup on a fresh checkout of
ui-classic, the migrate_database step would fail because it would run in
the spec/manageiq directory as opposed to the plugin root directory.

What happens is that the spec/manageiq directory doesn't have a
Gemfile.lock, because the bundle install happens in the plugin root, and
so when migrate_database runs db:migrate from the spec/manageiq
directory it fails saying it can't find certain gems.

This commit changes it to run db:migrate from the plugin root directory.
Additionally, I've added a create_database step, which is a noop when
the database exists, but also simplifies brand new checkout runs.
@@ -24,8 +24,10 @@ def self.manageiq_plugin_update(plugin_root = nil, force_bundle_update: true)
ensure_config_files

unless ENV["CI"]
Copy link
Member Author

Choose a reason for hiding this comment

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

On a separate topic, we have defined a separate ENV var for this step in the core bin/setup, but I don't think it's available in the plugin's bin/setup.

ENV["SKIP_DATABASE_SETUP"] = "true" if ENV["CI"]

I'm going to do this in a followup, but it might be preferable to default that same variable in the methods above, and that would allow the caller to do something like SKIP_DATABASE_SETUP=false in a CI env if they want to have the development database setup anyway (for example in a cypress test suite)

@miq-bot
Copy link
Member

miq-bot commented Jul 21, 2023

Some comments on commit Fryguy@be9c3e6

lib/manageiq/environment.rb

  • ⚠️ - 102 - Detected puts. Remove all debugging statements.

@miq-bot
Copy link
Member

miq-bot commented Jul 21, 2023

Checked commit Fryguy@be9c3e6 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
1 file checked, 1 offense detected

lib/manageiq/environment.rb

  • ❗ - Line 102, Col 7 - Rails/Output - Do not write to stdout. Use Rails's logger if you want to log.

Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@agrare agrare merged commit ce5d9f3 into ManageIQ:master Jul 21, 2023
9 checks passed
@Fryguy Fryguy deleted the fix_migrate_from_plugins branch July 21, 2023 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants