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

QSFP uplink & Integer ports support #230

Merged
merged 8 commits into from
May 10, 2017
Merged

QSFP uplink & Integer ports support #230

merged 8 commits into from
May 10, 2017

Conversation

vranystepan
Copy link
Contributor

@vranystepan vranystepan commented May 8, 2017

Description

This pull request contains fixes for #216 and #228. Now it is possible to create LIGs will all supported Interconnect Modules / Uplink ports. Legacy functionality for Ten-Gig (X) and Downlink (D) ports has remained preserved. There is also a new functionality (requested in #228) which enables use of Integer ports (final relative value of uplink port).

Issues Resolved

Check List

  • New functionality includes testing.
    • All tests pass ($ rake test).
  • New functionality has been documented in the README if applicable.
    • New functionality has been thoroughly documented in the examples (please include helpful comments).
  • Changes are documented in the CHANGELOG.

@vranystepan
Copy link
Contributor Author

I will also check examples. perhaps it would be nice to put there an example.

@vranystepan vranystepan changed the title QSFP uplink ports support. Unit tests. Changelog. QSFP uplink & Integer ports support May 8, 2017
@vranystepan
Copy link
Contributor Author

I've extended example in API300:Synergy with the most complex scenario: multi-Frame config, multi-Frame Uplink Sets. It's quite standard Synergy configuration ;)

@jsmartt
Copy link
Collaborator

jsmartt commented May 9, 2017

Hey @vranystepan , is this ready to go on your end?

@vranystepan
Copy link
Contributor Author

@jsmartt yes it is. At least on my end 😄

Copy link
Contributor

@fgbulsoni fgbulsoni left a comment

Choose a reason for hiding this comment

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

Looks good to me. We've got the changelog, examples, tests and updated methods here. Coverage seems unchanged as well.
@jsmartt @tmiotto Any extra thoughts?

Copy link
Collaborator

@jsmartt jsmartt left a comment

Choose a reason for hiding this comment

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

Looks great! 👍

# @param [String] interconnect model name
# @param [Fixnum] enclosure number for multi-frame configurations
def add_uplink(bay, port, type = nil, enclosure_index = 1)
enclosure_index = !type.nil? && type.include?('Virtual Connect SE 16Gb FC Module') ? -1 : enclosure_index
Copy link
Contributor

Choose a reason for hiding this comment

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

!type.nil? can be replaced in this clause by type

if type
fetch_relative_value_of(port, type)
else
port.to_s == port.to_i.to_s ? port : relative_value_of(port)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the point of port.to_s == port.to_i.to_s ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tmiotto I've put it there to handle for example 67 and "67" I can change it to port.is_a?(Integer) and accept only Integers for the "Integer" ports. 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.

Nope, it's fine as is! 😄

I would just add a comment explaining it, since it's not quite clear at first sight.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tmiotto cool. I've put there a comment. Also, such ports (67 or '67') will be always converted to Integer.

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.

None yet

4 participants