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

Singularize plural model #16833

Merged
merged 3 commits into from
Feb 16, 2018

Conversation

bdunne
Copy link
Member

@bdunne bdunne commented Jan 16, 2018

The model was created as a plural, luckily the table name appears to be correct so we don't need a schema migration.

Added in #14749
Found because of #16768 (comment)

@miq-bot
Copy link
Member

miq-bot commented Jan 16, 2018

Checked commits bdunne/manageiq@4f83aa7~...e7fbe82 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 2 offenses detected

app/models/asset_detail.rb

app/models/ems_refresh/save_inventory_physical_infra.rb

@Fryguy
Copy link
Member

Fryguy commented Jan 17, 2018

LOL @ the branch name

@Fryguy
Copy link
Member

Fryguy commented Jan 17, 2018

Have you verified this is not called in other repos? In particular in the Lenovo repo.

@agrare
Copy link
Member

agrare commented Jan 17, 2018

Its the first time I've seen that but it looks like it uses that as the base for the parsed result hash, so yes that would need to change otherwise you'll get save_inventory errors.

@bdunne
Copy link
Member Author

bdunne commented Jan 18, 2018

Okay @Fryguy I think I found all of the other repos that use this and linked the PR's here

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.

👍

@agrare agrare self-assigned this Jan 23, 2018
@blomquisg
Copy link
Member

@rodneyhbrown7 this change might impact developers that have local uncommitted changes for any features currently in development.

@agrare agrare assigned Fryguy and unassigned agrare Jan 30, 2018
@agrare
Copy link
Member

agrare commented Jan 30, 2018

Changing assignee to fryguy since I can't merge the ui-classic or api PRs together with this one

@Fryguy Fryguy merged commit a78bd64 into ManageIQ:master Feb 16, 2018
@Fryguy Fryguy added this to the Sprint 80 Ending Feb 26, 2018 milestone Feb 16, 2018
@Fryguy Fryguy added the bug label Feb 16, 2018
@bdunne bdunne deleted the asset_detailsssssssssssssssssssssss branch February 16, 2018 20:34
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.

5 participants