Skip to content

Conversation

@KevinLoiseau
Copy link
Contributor

Hi !

This PR fix the #save method on an existing security groups.

Thanks in advance for your time

@KevinLoiseau KevinLoiseau force-pushed the fix/network_security_group_update branch 2 times, most recently from 5d6dbab to ebcfb56 Compare October 31, 2018 10:10
@KevinLoiseau KevinLoiseau force-pushed the fix/network_security_group_update branch from ebcfb56 to 6d1c7f6 Compare October 31, 2018 10:15
@abbas-sheikh-confiz
Copy link
Contributor

Hi @KevinLoiseau ,
Please describe the scenario and steps to re-produce this error. We want to validate before we merge.

Thanks,
Abbas Sheikh

@KevinLoiseau
Copy link
Contributor Author

HI @abbas-sheikh-confiz, thank you to take care of this.

My scenario is:

client = Fog::Network.new(
  :provider => 'AzureRM',
  :tenant_id => "XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX",
  :client_id => "XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX",
  :client_secret => "XXXXXXXXXXXXXXXXXXXXXXXXXX",
  :subscription_id => "XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX"                                                                                                                                                                                   
)

sg = client.network_security_groups.get('RG_NAME', 'SG_NAME')
sg.tags = { 'tag' => 'value' }
sg.save # => without this PR; raise "ArgumentError: Parameter security_rules must be an Array of Hashes"

Regards,
Kevin Loiseau

@maham-nazir-confiz
Copy link
Contributor

@KevinLoiseau
Please make the following changes to your PR

  • Write a unit test for the new method that you have written
  • Update the network_security_group.rb integration test to test out your scenario as well.

Thank you.
Regards

@maham-nazir-confiz
Copy link
Contributor

@KevinLoiseau

Update the network_security_group.rb integration test to test out your scenario as well.

Thank you.
Regards

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants