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

Default connection_status to connected #18216

Merged
merged 1 commit into from
Nov 19, 2018

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Nov 16, 2018

For most providers, connection_status is populated
For cloud providers, it is sometimes left to be nil.
Since it doesn't make sense.

But nil for them actually mean connected.

So this changes what nil means.
We're also modifying the parsers for the providers to the nil
will not get into the database.

Related to:

Fixes: ManageIQ/manageiq-ui-classic#4909

/cc @agrare @miha-plesko

For most providers, connection_status is populated
For cloud providers, it is sometimes left to be nil.
Since it doesn't make sense.

But nil for them actually mean connected.

So this changes what nil means.
We're also modifying the parsers for the providers to the nil
will not get into the database.
@miq-bot
Copy link
Member

miq-bot commented Nov 16, 2018

Checked commit kbrock@1b22b67 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. 🍰

@miha-plesko
Copy link
Contributor

Hm, isn't this almost the opposite of what we were talking about earlier when we said nil value should not be interpreted? I don't mind the change, but by ensuring non nullable value this patch should not be needed actually.

@kbrock
Copy link
Member Author

kbrock commented Nov 17, 2018

@miha-plesko Yes, I like not interpreting nil to mean something special. @agrare modified all the parsers, and we'll need to write a migration.

In the mean time, we need to interpret nil properly.
This is what I thought @agrare wanted, but he can just close if he had other ideas in mind

@miha-plesko
Copy link
Contributor

@agrare @kbrock I think we should close this PR and proceed like this:

Quickly fix Hammer issue for vCloud with minimum impact on other providers:

Properly solve the problem on master only:

WDYT?

@agrare
Copy link
Member

agrare commented Nov 19, 2018

Hey @miha-plesko

Quickly fix Hammer issue for vCloud with minimum impact on other providers:

ManageIQ/manageiq-providers-vmware#339 (merge master + backport)

👍 I agree with this

Properly solve the problem on master only:

#18213 (merge, no backport)

I prefer this PR over that one because changing the default_value_for won't help for existing vms but yes I agree we should migrate nils => connected and update the parsers so we don't need this.

@kbrock
Copy link
Member Author

kbrock commented Nov 19, 2018

@miha-plesko I'm concerned that the search functionality won't work because we'll be looking for cloud vms and they won't show up because nil is coded to mean disconnected (but in truth, nil means connected for cloud).

That is my driving reason for this PR and for joining this conversation

@agrare agrare merged commit 36a6761 into ManageIQ:master Nov 19, 2018
@agrare agrare added this to the Sprint 99 Ending Nov 19, 2018 milestone Nov 19, 2018
@miha-plesko
Copy link
Contributor

@miq-bot remove_label hammer/yes

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.

4 participants