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

Add methods for for provider creation pluggable UI #149

Merged

Conversation

agrare
Copy link
Member

@agrare agrare commented Oct 2, 2019

Add a method indicating the properties required to create a Kubevirt
provider and implement the common self.verify_credentials interface
method.

ManageIQ/manageiq#18818

:name => "endpoints.default.port",
:type => "number",
:isRequired => true,
:validate => [{:type => "required-validator"}]
Copy link
Member

Choose a reason for hiding this comment

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

I think there's a validator for numbers.

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean for a port range like [1, 65535]? Yeah can add that

@agrare agrare force-pushed the add_params_for_create_and_verify_credentials branch from 6766ad4 to e89531c Compare October 3, 2019 23:17
:isRequired => true,
:validate => [
{:type => "required-validator"},
{:type => "validatorTypes.MIN_NUMBER_VALUE",
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if this is right, @Hyperkid123 ?

Choose a reason for hiding this comment

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

Yes that seems correct.

You can see live example with same use case just different values here if you want to see it live: https://data-driven-forms.org/renderer/validators#numbervaluevalidators

Copy link
Member

@Fryguy Fryguy Oct 4, 2019

Choose a reason for hiding this comment

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

What's with the => spacing? Didn't realize you were aligning to the stuff after...even so, I think the

{:type  => "a",
 :value => 1
}

is a weird layout, particularly because it introduce the single space...would prefer

{
  :type  => "a",
  :value => 1
}

Copy link
Member Author

Choose a reason for hiding this comment

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


def self.verify_credentials(args)
server, port, token = args.dig("endpoints", "default")&.values_at("server", "port", "token")
!!raw_connect(:server => server, :port => port, :token => token)
Copy link
Member

Choose a reason for hiding this comment

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

Super minor but if the incoming hash already has the keys...this can be simplified to just use slice.

!!raw_connect(args.dig("endpoints", "default")&.slice("server", "port", "token").symbolize_keys)

Copy link
Member

Choose a reason for hiding this comment

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

Also, interestingly, the docs for raw_connect say it takes verify_ssl and ca_certs, but then never uses them? https://github.com/ManageIQ/manageiq-providers-kubevirt/pull/149/files#diff-ff17b05a7c525eec833f0ddb0134a0d9R109-R111

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I couldn't tell if that was an oversight or not so I left the description for now.

@agrare agrare force-pushed the add_params_for_create_and_verify_credentials branch 2 times, most recently from 4318df1 to d6b4f70 Compare October 4, 2019 16:15
Add a method indicating the properties required to create a Kubevirt
provider and implement the common self.verify_credentials interface
method.

ManageIQ/manageiq#18818
@agrare agrare force-pushed the add_params_for_create_and_verify_credentials branch 2 times, most recently from 72f3a68 to 02e4ba3 Compare October 7, 2019 16:38
@agrare agrare force-pushed the add_params_for_create_and_verify_credentials branch from 02e4ba3 to 8f7439d Compare October 7, 2019 16:58
@miq-bot
Copy link
Member

miq-bot commented Oct 7, 2019

Checked commits agrare/manageiq-providers-kubevirt@b1235ac~...8f7439d with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🏆

@Fryguy Fryguy merged commit 2c0567b into ManageIQ:master Oct 8, 2019
@Fryguy Fryguy added this to the Sprint 122 Ending Oct 14, 2019 milestone Oct 8, 2019
@agrare agrare deleted the add_params_for_create_and_verify_credentials branch October 8, 2019 18:00
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