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.

Sign up for GitHub

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

Jump to bottom

Parsing the connection b/w ports #194

Merged
merged 1 commit into from
Jul 12, 2018
Merged

Parsing the connection b/w ports #194

merged 1 commit into from
Jul 12, 2018

Conversation

douglasgabriel
Copy link
Member

@douglasgabriel douglasgabriel commented May 29, 2018 •
edited
Loading

This PR is able to

  • Fill the switches physical ports uid_ems;
  • Bind the ports from Physical Servers and Physical Switches;

Informations
A port is connected to another if its MAC address is described in the peer_mac_address of the other port.

Depends on
ManageIQ/manageiq-schema#208 - Merged
ManageIQ/manageiq#17311 Merged

@douglasgabriel douglasgabriel force-pushed the binding_physical_ports branch from 0eee543 to 3986386 Compare May 30, 2018 11:59
@douglasgabriel
Copy link
Member Author

@miq-bot assign @agrare

@douglasgabriel douglasgabriel force-pushed the binding_physical_ports branch from 3986386 to 9b42359 Compare May 30, 2018 19:17
@miq-bot
Copy link
Member

miq-bot commented Jun 13, 2018

This pull request is not mergeable. Please rebase and repush.

@douglasgabriel douglasgabriel force-pushed the binding_physical_ports branch from 9b42359 to 2c7cfb8 Compare July 2, 2018 12:32
@agrare agrare removed the unmergeable label Jul 2, 2018
@douglasgabriel douglasgabriel force-pushed the binding_physical_ports branch from 2c7cfb8 to ad3e842 Compare July 2, 2018 17:43
@agrare agrare assigned rodneyhbrown7 and unassigned agrare Jul 2, 2018
agrare
agrare reviewed Jul 2, 2018
View reviewed changes
spec/manageiq-schema Outdated
@@ -0,0 +1 @@
/home/douglas/workspace/manageiq-schema
Copy link
Member

@agrare agrare Jul 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Delete this, have to be careful with git commit -a --amend 😄

@agrare
Copy link
Member

agrare commented Jul 2, 2018

@rodneyhbrown7 assigning to you to review, looks fine from the MIQ side but need you to review the LXCA stuff

@douglasgabriel douglasgabriel force-pushed the binding_physical_ports branch from ad3e842 to 4abe17f Compare July 2, 2018 17:52
@miq-bot
Copy link
Member

miq-bot commented Jul 2, 2018

Checked commit douglasgabriel@4abe17f with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 0 offenses detected
Everything looks fine. 🍰

@agrare
Copy link
Member

agrare commented Jul 10, 2018

@douglasgabriel or if you can get a review from another member of your team I'll merge this given how long it has been sitting here

@douglasgabriel
Copy link
Member Author

@caiocmpaes could you review that, please?

caiocmpaes
caiocmpaes approved these changes Jul 11, 2018
View reviewed changes
Copy link
Contributor

@caiocmpaes caiocmpaes left a comment

Choose a reason for hiding this comment

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

LGTM 👍

I tested mocking data of PhysicalNetworkPorts and the connection is created with success.

I only had a suggestion about the naming of one method. You can see It on the inline comments.

app/models/manageiq/providers/lenovo/physical_infra_manager/refresh_parser.rb
@@ -139,6 +141,18 @@ def get_config_patterns
config_patterns = @connection.discover_config_pattern
config_patterns.map { |config_pattern| @parser.parse_config_pattern(config_pattern) }
end

def bind_network_ports(inventory)
Copy link
Contributor

Choose a reason for hiding this comment

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

As the method changes the inventory hash, shouldn't It be bind_network_ports! (with the !)? The same way you put on the parser method.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, @caiocmpaes, it's a fair concern, but our methods in this class doesn't use this pattern and to keep the consistence I would say to keep this name. Also the method that is really doing the changes has already its name ending with !. What do you think? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. I Agree with you. Let It be the current name, then. 👍

@agrare agrare merged commit 4abe17f into ManageIQ:master Jul 12, 2018
agrare added a commit that referenced this pull request Jul 12, 2018
@agrare
Merge pull request #194 from douglasgabriel/binding_physical_ports
bcebe8e 
Parsing the connection b/w ports
@agrare agrare added this to the Sprint 90 Ending Jul 16, 2018 milestone Jul 12, 2018
@agrare
Copy link
Member

agrare commented Jul 12, 2018

Thanks @douglasgabriel really awesome enhancement from schema all the way to the parser

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caiocmpaes caiocmpaes caiocmpaes approved these changes

@agrare agrare agrare approved these changes

Assignees

@rodneyhbrown7 rodneyhbrown7

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@douglasgabriel @miq-bot @agrare @caiocmpaes @rodneyhbrown7