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

Update PowerVS cloud subnet parser #77

Merged

Conversation

jaywcarman
Copy link
Member

  • Add AZ relationship
  • Save network type to 'extra_attributes' column

@agrare
Copy link
Member

agrare commented Oct 7, 2020

Looks good @jaywcarman can you add a check in the assert_specific_cloud_subnet spec? Something like expect(cloud_subnet.availability_zone&.ems_ref).to eq(ems.uid_ems)

:ip_version => '4',
:network_protocol => 'IPv4',
:availability_zone => persister.availability_zones.lazy_find(persister.cloud_manager.uid_ems),
:extra_attributes => network['type']
Copy link
Member Author

Choose a reason for hiding this comment

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

@agrare Should I store this as a hash {'type' => network['type']} ?

We weren't using it before, but this is in our current refresher spec:
https://github.com/ManageIQ/manageiq-providers-ibm_cloud/blob/master/spec/models/manageiq/providers/ibm_cloud/power_virtual_servers/cloud_manager/refresher_spec.rb#L152

Copy link
Member Author

@jaywcarman jaywcarman Oct 7, 2020

Choose a reason for hiding this comment

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

Do all extra attributes passed to persister.cloud_subnets.build automatically get added to the cloud_subnets extra_attributes DB column?

Copy link
Member

Choose a reason for hiding this comment

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

That's interesting, wasn't aware of this but we define some accessor methods that use extra_attributes as the backing: https://github.com/ManageIQ/manageiq/blob/master/app/models/cloud_subnet.rb#L35-L43
so cloud_subnet.ip_version= gets saved to extra_attributes

In this case maybe it is best to define a network_type= which saves it to extra_attribtues

I learn something new in this huge product everyday

Copy link
Member Author

@jaywcarman jaywcarman Oct 7, 2020

Choose a reason for hiding this comment

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

:) I know the feeling, that's for sure! I was very confused as to how the specs were passing without any existing extra_attributes, haha.

I agree with defining cloud_subnet.network_type= so we can keep ip_version. I've noticed the providers are inconsistent on using IPv4 vs ipv4 for network protocol so I could see it being useful.

Copy link
Member Author

Choose a reason for hiding this comment

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

- Add AZ relationship
- Save network type to 'extra_attributes' column
@miq-bot
Copy link
Member

miq-bot commented Oct 8, 2020

Checked commit jaywcarman@61d8eec with ruby 2.6.3, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. 🍪

@jaywcarman
Copy link
Member Author

Cross repo test with ManageIQ/manageiq#20672 is passing:
ManageIQ/manageiq-cross_repo-tests#188

@agrare agrare closed this Oct 8, 2020
@agrare agrare reopened this Oct 8, 2020
@agrare agrare self-assigned this Oct 8, 2020
@agrare agrare added the enhancement New feature or request label Oct 8, 2020
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.

👍 awesome @jaywcarman !

@agrare agrare merged commit bd38461 into ManageIQ:master Oct 8, 2020
@jaywcarman jaywcarman deleted the powervs_update_cloud_subnet_parser branch October 8, 2020 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants