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

Updating details for physical switches #182

Merged
merged 1 commit into from
Apr 17, 2018

Conversation

saulotoledo
Copy link
Member

@saulotoledo saulotoledo commented Mar 23, 2018

Removes details from switches table, since due to later changes we can now use asset_details table to store the same information. We also add the part_number column to the asset details and rename the relationship in the hardwares table, since we will use physical_switches as the model in the core PR.

@saulotoledo
Copy link
Member Author

@bdunne, @agrare, could you take a look in the lint?

I think I should not have to fix the Style/MethodCallWithArgsParentheses rule above. There is no commit in the project by using parentheses. Here is an example.

@agrare
Copy link
Member

agrare commented Mar 27, 2018

@saulotoledo don't worry about that something must be wrong with the rubocop exceptions in this repo, /cc @bdunne

@bdunne
Copy link
Member

bdunne commented Mar 27, 2018

That's correct, I updated the ignored methods list in ManageIQ/guides#299

@saulotoledo saulotoledo changed the title [WIP] Removing switch details [WIP] Updating details for physical switches Mar 27, 2018
@saulotoledo saulotoledo changed the title [WIP] Updating details for physical switches Updating details for physical switches Mar 27, 2018
@saulotoledo
Copy link
Member Author

@rodneyhbrown7, @agrare, @bdunne, I think this is ok for review.

@@ -0,0 +1,5 @@
class RenameSwitchIdColumnInHardware < ActiveRecord::Migration[5.0]
def change
rename_column :hardwares, :switch_id, :physical_switch_id
Copy link
Member

Choose a reason for hiding this comment

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

You should be fine with switch_id here since the table is still called switches the fk should be switch_id. Physical_switches is just the name of the association

Copy link
Member Author

@saulotoledo saulotoledo Apr 5, 2018

Choose a reason for hiding this comment

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

@agrare Done

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

@miq-bot
Copy link
Member

miq-bot commented Apr 5, 2018

Checked commit saulotoledo@055aa80 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

@gtanzillo
Copy link
Member

@Fryguy You ok with merging this one?

@Fryguy
Copy link
Member

Fryguy commented Apr 11, 2018

@saulotoledo Is there a corresponding PR where the callers of these columns are changed such that they are no longer used? I'm concerned that if I merge, then master will be broken.

@agrare
Copy link
Member

agrare commented Apr 11, 2018

@Fryguy ManageIQ/manageiq-providers-lenovo#132 updates the parser to work with the new columns

@Fryguy
Copy link
Member

Fryguy commented Apr 11, 2018

@agrare So that means that ManageIQ/manageiq-providers-lenovo#132 needs to merged in tandem with this one?

@saulotoledo
Copy link
Member Author

saulotoledo commented Apr 11, 2018

@Fryguy @agrare It is safe to merge this one. The only code who uses these columns is ManageIQ/manageiq-providers-lenovo#132, so this one needs to be merged first. These colums were created by me on #160 and are only used in my PRs (provider and UI on WIP state yet).

Also, there is no need to merge them at the same time, because there is no code in the provider that can break.

@gtanzillo
Copy link
Member

@saulotoledo We're (me and @Fryguy) ready to merge this PR but, we just need to know for certain that the columns that you're removing here are NOT referenced in any other code, including ManageIQ/manageiq-providers-lenovo#132

+    remove_column :switches, :product_name,  :string
+    remove_column :switches, :part_number,   :string
+    remove_column :switches, :serial_number, :string
+    remove_column :switches, :description,   :string
+    remove_column :switches, :manufacturer,  :string

Please confirm, thanks

@saulotoledo
Copy link
Member Author

@gtanzillo @Fryguy @rodneyhbrown7 Yes, I confirm that.

The long answer: I created them to work in that PR only, but the same colums were created into asset_details table in another PR meanwhile. I just updated the ManageIQ/manageiq-providers-lenovo#132 code to use the asset_details table and those columns are not necessary anymore. There is no other place using them, not even the ManageIQ/manageiq-providers-lenovo#132 code.

@Fryguy
Copy link
Member

Fryguy commented Apr 17, 2018

@saulotoledo I guess what Greg and I are asking is, if we merge the schema change, does the code in master currently use those removed columns, which could lead to a crash on the master? Or, do I have to merge ManageIQ/manageiq-providers-lenovo#132 into master simulatenously as I merge the schema change?

@rodneyhbrown7
Copy link

rodneyhbrown7 commented Apr 17, 2018

@Fryguy There is currently no code that is using the columns that are being removed. Those columns were added but no code was developed to put data into the column.

ManageIQ/manageiq-providers-lenovo#132 is dependent on the addition of the asset_details column in this PR. It was decided to add the data to that table instead of adding the data to the native columns here, so we are requesting removal of the unused columns.

@saulotoledo
Copy link
Member Author

@Fryguy @gtanzillo Yes, I am trying to say what @rodneyhbrown7 said. Sorry for the misunderstanding.

@Fryguy
Copy link
Member

Fryguy commented Apr 17, 2018

Great. Thanks for the clarification.

@Fryguy Fryguy merged commit 3679c30 into ManageIQ:master Apr 17, 2018
@Fryguy Fryguy added this to the Sprint 84 Ending Apr 23, 2018 milestone Apr 17, 2018
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.

7 participants