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

Adds cloud_services table #8093

Merged
merged 1 commit into from
Apr 19, 2016
Merged

Conversation

petrblaho
Copy link

@petrblaho petrblaho commented Apr 19, 2016

This table will be used for CloudService model.

EDIT (@blomquisg): Pulled from #7996

t.string :binary
t.string :hostname
t.string :status
t.boolean :scheduling
Copy link
Contributor

Choose a reason for hiding this comment

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

probably adding scheduling_enabled or disabled, based on what it means

Copy link
Author

@petrblaho petrblaho Apr 19, 2016

Choose a reason for hiding this comment

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

We have scheduling_disabled_reason there too... so this push me to using scheduling_disabled but I think scheduling_enabled seems more appropriate.... what do you think?

Update: renamed to scheduling_disabled

@petrblaho petrblaho force-pushed the cloud-service-schema branch from 396608f to 859d8c7 Compare April 19, 2016 15:35
@Ladas
Copy link
Contributor

Ladas commented Apr 19, 2016

maybe I would call it host_name and zone_name, but it can be like it is now. Looks good to me now 👍

create_table :cloud_services do |t|
t.string :ems_ref
t.string :source
t.string :binary
Copy link
Member

Choose a reason for hiding this comment

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

What are source and binary? These names are too ambiguous right now. If you give me a decent explanation, I could help with a better column name.

Copy link
Member

@Fryguy Fryguy Apr 19, 2016

Choose a reason for hiding this comment

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

Expanations are here for source and here for binary. I have a comment on binary below in this PR thread

@blomquisg
Copy link
Member

@petrblaho I think I asked this in another PR (not sure which)...

What's the difference between a cloud_service and a system_service?

In other words, could the fields being added to this new cloud_service table, just be added to system_service and we use that instead?

I'm not sure I understand the distinction here.

@blomquisg
Copy link
Member

What's the difference between a cloud_service and a system_service?

Answering my own question...

Just talked to @Ladas about this. He said that cloud_service details actually come from the OSP Cloud REST API. While the system_service details come from the the OSP Infra SSA.

We would have a CloudService available on the cloud side regardless of whether the there's a corresponding OSP Infra connected to provide the SystemService.

@petrblaho
Copy link
Author

@blomquisg I think answers for your inline questions can be found in discussion at #7996 - @Fryguy already asked similar questions.

And as I can see you already has the answer for CloudService vs. SystemService.

@petrblaho
Copy link
Author

@blomquisg zonename, hostname and binary are coming from Fog Compute Service model - so I need to store that to match CloudService with AvailabilityZone, Host and SystemService from MIQ models.

@Fryguy
Copy link
Member

Fryguy commented Apr 19, 2016

Aside from my comments, everything else looks fine, though I do question the binary column name. I get that it matches fog's interface, but when I hear "binary" in a database I think of the binary column type...I'm not thinking about an executable on the command line. Maybe executable or cli might be a better name choice. @Ladas @blomquisg thoughts?

EDIT: My thoughts on binary might be tainted by the fact that we already have a method called binary on the binary_blobs table, but maybe that's just me 😉

@petrblaho
Copy link
Author

@Fryguy executable_name - what do you think?

And I should probably rename zonename to availability_zonename...

@blomquisg
Copy link
Member

@Fryguy executable_name - what do you think?

I like this better.

And I should probably rename zonename to availability_zonename...

availability_zone_name , maybe? ... I know it's inconsistent with hostname ...

@Fryguy
Copy link
Member

Fryguy commented Apr 19, 2016

yeah exectuable_name sounds great...very clear.

@blomquisg
Copy link
Member

Summary of a conversation from https://gitter.im/ManageIQ/manageiq/providers

  • Keep hostname because the user wants to see what host the service is on, and there might not be an underlying OSP Infra layer to dereference cloud_service.host.hostname.
  • Remove availability_zone_name because this is a cloud object, and should definitely have a relationship in cloud_service, so cloud_service.availability_zone.name should always work.
  • Rename binary to executable_name for clarity.

Thanks @petrblaho !!

@petrblaho petrblaho force-pushed the cloud-service-schema branch from 859d8c7 to d6bb43b Compare April 19, 2016 16:34
@Fryguy
Copy link
Member

Fryguy commented Apr 19, 2016

Looks great @petrblaho ...if you could align the symbols for style, then this is good to go.

This table will be used for CloudService model.
@petrblaho petrblaho force-pushed the cloud-service-schema branch from d6bb43b to c99b526 Compare April 19, 2016 16:37
@Fryguy
Copy link
Member

Fryguy commented Apr 19, 2016

LGTM 👍 will merge when green.

@miq-bot
Copy link
Member

miq-bot commented Apr 19, 2016

Checked commit petrblaho@c99b526 with ruby 2.2.3, rubocop 0.37.2, and haml-lint 0.16.1
1 file checked, 0 offenses detected
Everything looks good. 👍

@Fryguy Fryguy merged commit 35a2f08 into ManageIQ:master Apr 19, 2016
@Fryguy Fryguy added this to the Sprint 39 Ending Apr 19, 2016 milestone Apr 19, 2016
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.

6 participants