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

Profile description #254

Merged
merged 1 commit into from May 30, 2018
Merged

Profile description #254

merged 1 commit into from May 30, 2018

Conversation

AlonaKaplan
Copy link
Contributor

Accept 'profile_name (network_name)' as a valid vlan of vm provision

'profile_id' or 'profile_name (network_name)' are both acceptable.

Fixes https://bugzilla.redhat.com/1574351

@AlonaKaplan
Copy link
Contributor Author

@borod108 please review

@AlonaKaplan AlonaKaplan force-pushed the profile_description branch 2 times, most recently from 8efd515 to f15b7bb Compare May 28, 2018 09:54
@mwperina
Copy link

@pkliczewski Piotr, could you please review and Boris is in the training?

borod108
borod108 reviewed May 29, 2018
View reviewed changes
spec/models/manageiq/providers/redhat/infra_manager/provision/configuration/network_spec.rb Outdated
end

def get_profile_description(vnic_profile_name, network_name)
vnic_profile_name + " (" + network_name + ")"
Copy link
Contributor

Choose a reason for hiding this comment

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

"#{vnic_profile_name} (#{network_name})"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

spec/models/manageiq/providers/redhat/infra_manager/provision/configuration/network_spec.rb Outdated
expect(@task.get_mac_address_of_nic_on_requested_vlan).to eq(mac_address)
end

it 'nics list does not contain a nic with the specified profile description' do
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand the test correctly, the "it" statement is actually a context - "it" should specify what is the expected result, while context describe in what context it happens. (same for previous "it")

@borod108
Copy link
Contributor

@AlonaKaplan also codeclimet if possible...

@pkliczewski
Copy link
Contributor

@AlonaKaplan @mwperina The code looks good. Let's fix codeclimate issues as @borod108 suggested.

@AlonaKaplan
Copy link
Contributor Author

Regarding the codeclimate.

There are two issues -

  1. Method ws_network_fields has a Cognitive Complexity of 9 (exceeds 5 allowed).
    The method is pretty simple and small. Do you have an idea why its complexity is 9? Do you think it worth refactoring it?
  2. Useless assignment to variable - dialog_name. The variable is used in the line after the assignment, I don't know why codeclimate says it is useless and how I can fix it.

@JPrause
Copy link
Member

JPrause commented May 29, 2018

@miq-bot add_label blocker

@AlonaKaplan AlonaKaplan force-pushed the profile_description branch 2 times, most recently from e1a6c4e to 05354c6 Compare May 30, 2018 07:26
borod108
borod108 reviewed May 30, 2018
View reviewed changes
app/models/manageiq/providers/redhat/infra_manager/ovirt_services/strategies/v4.rb
end

def parse_vnic_profile_id(requested_profile, uid_ems_cluster)
if requested_profile.include?('(')
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please give a name to this? Because I am not sure it will be clear what having '(' means.

app/models/manageiq/providers/redhat/infra_manager/ovirt_services/strategies/v4.rb
vlans = {}
private_load_allowed_networks(vlans, uid_ems_cluster)
matches = vlans.select { |_profile_id, profile_description| profile_description == requested_profile }
return matches.keys[0] unless matches.empty?
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use safe navigation?

@AlonaKaplan
Accept 'profile_name (network_name)' as a valid vlan for vm provision
49f086a 
'profile_id' or 'profile_name (network_name)' are both acceptable.

Fixes https://bugzilla.redhat.com/1574351
@miq-bot
Copy link
Member

miq-bot commented May 30, 2018

Checked commit AlonaKaplan@49f086a with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🏆

@borod108 borod108 merged commit 1c70679 into ManageIQ:master May 30, 2018
simaishi pushed a commit that referenced this pull request May 30, 2018
@borod108 @simaishi
Merge pull request #254 from AlonaKaplan/profile_description
946a17f 
Profile description
(cherry picked from commit 1c70679)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1584406
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit 946a17fe35a444a26113a15c463ff0f95179771f
Author: Boris Od <boris.od@gmail.com>
Date:   Wed May 30 15:07:03 2018 +0300

    Merge pull request #254 from AlonaKaplan/profile_description
    
    Profile description
    (cherry picked from commit 1c70679ac8b5ffb5163df217848c98447bc8a795)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1584406

@agrare agrare added this to the Sprint 87 Ending Jun 4, 2018 milestone Jun 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@borod108 borod108 borod108 left review comments

Assignees

@borod108 borod108

Labels
blocker gaprindashvili/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@AlonaKaplan @mwperina @borod108 @pkliczewski @JPrause @miq-bot @simaishi @agrare