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

Cloud VM should be marked as connected by default #18213

Conversation

miha-plesko
Copy link
Contributor

With this commit we set default value of connection stats for cloud instances, which is supposed to be 'connected' for all of them. That's because cloud instances can not reach disconnected state. Until now connection_stats = nil was the default which is interpreted as 'disconnected' by MIQ -> which is imposssible for cloud instances per se.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1649403
Fixes ManageIQ/manageiq-ui-classic#4909

@miq-bot assign @agrare
@miq-bot add_label enhancement,hammer/yes

/cc @kbrock @Fryguy @skateman

With this commit we set default value of connection stats for
cloud instances, which is supposed to be 'connected' for all
of them. That's because cloud instances can not reach disconnected
state. Until now `connection_stats = nil` was the default which is
interpreted as 'disconnected' by MIQ -> which is imposssible for
cloud instances per se.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1649403
Fixes ManageIQ/manageiq-ui-classic#4909

Signed-off-by: Miha Pleško <miha.plesko@xlab.si>
Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

LGTM

@kbrock
Copy link
Member

kbrock commented Nov 16, 2018

Thanks @miha-plesko This wasn't your initial solution. I appreciate you hearing me out and putting this PR together.

@agrare can decide which one works best for the providers as a whole

@miha-plesko
Copy link
Contributor Author

Sorry guys for opening the 3rd PR on this topic today - I'm struggling between fixing it for vCloud only or fixing it in general and risk breaking other providers :)

I kind of like the solution @kbrock suggested. Nice thing is also that rspecs will explode in all cloud providers (where we assert nil) so we'll know what all providers need a cleanup. Once we merge one of the two offered core PRs, we can do followup cleanup PRs on all cloud providers.

Copy link
Member

@skateman skateman left a comment

Choose a reason for hiding this comment

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

The Seal of Approval

@miq-bot
Copy link
Member

miq-bot commented Nov 16, 2018

Checked commit miha-plesko@dd0ff97 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

@agrare
Copy link
Member

agrare commented Nov 16, 2018

@miha-plesko this will work for newly created VMs but we would need a migration to fix existing VMs as those will still show disconnected.

For a lower risk change I'd recommend overriding disconnected(?) in vcloud vm like we do for amazon/azure/google. This can be a lower risk for backport. Then I think we should fix the method and arel in core to behave like a normal ? boolean method and return false on nil.

FTR I think a data migration to update all null connection_states to "connected" and changing the parsers to set it always is a good idea so we don't have unset values in the db.

@miha-plesko
Copy link
Contributor Author

Cool, let me open a separate PR on vcd as you suggest so we needn't backport this one.

@miq-bot remove_label hammer/yes

@miha-plesko
Copy link
Contributor Author

Closing in favor of #18216 accompanied with provider inventory updates where we'll explicitly set connection_state to connected.

@miha-plesko miha-plesko deleted the cloud-instance-is-always-connected branch January 7, 2019 08:21
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.

5 participants