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

Remove RSS table #343

Merged
merged 1 commit into from
May 1, 2019
Merged

Remove RSS table #343

merged 1 commit into from
May 1, 2019

Conversation

juliancheal
Copy link
Member

@juliancheal juliancheal commented Mar 1, 2019

Follow on from ManageIQ/manageiq#18478

@@ -0,0 +1,5 @@
class RemoveRssFeeds < ActiveRecord::Migration[5.0]
def change
drop_table :rss_feeds
Copy link
Member

Choose a reason for hiding this comment

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

The cop is complaining because this isn't reversible? If we don't want to put the table in the down migration, maybe we should write an up without a down.... but why not just recreate it as it is now in the down?

Copy link
Member

Choose a reason for hiding this comment

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

The reverse is required.

@miq-bot
Copy link
Member

miq-bot commented Mar 19, 2019

Checked commit juliancheal@4b4e72a with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 1 offense detected

db/migrate/20190301174502_remove_rss_feeds.rb

Copy link
Member

@jrafanie jrafanie left a comment

Choose a reason for hiding this comment

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

LGTM, please merge @Fryguy if good for you

@juliancheal
Copy link
Member Author

Presumably we can ignore this cop?

@juliancheal
Copy link
Member Author

@miq-bot assign @Fryguy

@Fryguy Fryguy merged commit 07d8380 into ManageIQ:master May 1, 2019
@Fryguy Fryguy added this to the Sprint 111 Ending May 13, 2019 milestone May 1, 2019
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.

4 participants