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

Added hostname fallback fields for openstack amqp events. #5270

Merged
merged 1 commit into from
Mar 13, 2019

Conversation

Hyperkid123
Copy link
Contributor

@Hyperkid123 Hyperkid123 commented Feb 25, 2019

Solves: https://bugzilla.redhat.com/show_bug.cgi?id=1595558

Adds two new fallback text fields for hostname.
screenshot-localhost-3000-2019 02 25-16-02-28

@h-kataria @alexander-demichev do you know what labels should be assigned to these fields? Field names in model have been assigned based on this: https://github.com/ManageIQ/manageiq-providers-openstack/pull/394/files#diff-6f9b687ce51fd31aa102fa9305ef2501R148

@himdel @martinpovolny I would like to go and start converting this form into react data driven forms. Even though this was a small and relatively "safe" change i am afraid it introduces another technical debt and further diminishes maintainability. Your thoughts?

@miq-bot miq-bot added the wip label Feb 25, 2019
@himdel
Copy link
Contributor

himdel commented Feb 25, 2019

@himdel @martinpovolny I would like to go and start converting this form into react data driven forms. Even though this was a small and relatively "safe" change i am afraid it introduces another technical debt and further diminishes maintainability. Your thoughts?

The change looks safe enough, but without testing it I have about 0 confidence this won't break anywhere :). (Will test once no longer wip.)

It also copies twice a piece of form that's already there, so, you're right that this is not exactly technical debt free.

That said, given this needs to be backported, sounds like this is the only way to achieve that 👍

And I absolutely agree ems forms should be absolutely top priority for data driven forms, even if we have to provide all the data ourselves instead of the provider repos.

@Hyperkid123
Copy link
Contributor Author

The change looks safe enough, but without testing it I have about 0 confidence this won't break anywhere :). (Will test once no longer wip.)

Could not agree more

@Hyperkid123 Hyperkid123 force-pushed the openstack-provider branch 2 times, most recently from dfe8e2d to 92da180 Compare February 25, 2019 14:59
@Hyperkid123 Hyperkid123 changed the title [WIP] Added hostname fallback fields for openstack amqp events. Added hostname fallback fields for openstack amqp events. Feb 25, 2019
@miq-bot miq-bot removed the wip label Feb 25, 2019
@Hyperkid123
Copy link
Contributor Author

@miq-bot add-reviewer @h-kataria
@miq-bot add-reviewer @himdel

@miq-bot miq-bot requested review from h-kataria and himdel February 25, 2019 15:04
@h-kataria
Copy link
Contributor

@alexander-demichev can you test changes in this PR?

"ng-readonly" => ng_readonly_hostname,
"maxlength" => ViewHelper::MAX_NAME_LEN.to_s,
"ng-trim" => false,
"placeholder" => "Hostname or IPV4 or IPV6 address",
Copy link
Contributor

Choose a reason for hiding this comment

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

gettext

"ng-readonly" => ng_readonly_hostname,
"maxlength" => ViewHelper::MAX_NAME_LEN.to_s,
"ng-trim" => false,
"placeholder" => "Hostname or IPV4 or IPV6 address",
Copy link
Contributor

Choose a reason for hiding this comment

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

gettext

@miq-bot
Copy link
Member

miq-bot commented Feb 26, 2019

Checked commit Hyperkid123@58e43c5 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 🏆

@h-kataria
Copy link
Contributor

@alexander-demichev Can you please verify changes in this PR.

@h-kataria
Copy link
Contributor

@aufi can someone on your team help us test/verify this fix. Thanks.

@aufi
Copy link
Member

aufi commented Mar 13, 2019

I wasn't able test this end-to-end due to infrastructure complexity. But, new fields introduced in this PR look to be passed to backend, so looks good to me, 👍 from OpenStack side.

@h-kataria h-kataria self-assigned this Mar 13, 2019
@h-kataria h-kataria added this to the Sprint 107 Ending Mar 18, 2019 milestone Mar 13, 2019
@h-kataria h-kataria merged commit 25322f1 into ManageIQ:master Mar 13, 2019
@h-kataria
Copy link
Contributor

@aufi Thx for testing.

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