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 JSON-Schema create-params and a common Validate Credentials API #551

Merged
merged 10 commits into from
Sep 6, 2019

Conversation

agrare
Copy link
Member

@agrare agrare commented Aug 8, 2019

Add a set of parameters required by the UI to drive their data-driven-forms and implement the common validate_credentials API instead of raw_connect.

@agrare
Copy link
Member Author

agrare commented Aug 8, 2019

cc @skateman

@agrare agrare changed the title Add JSON-Schema create-params and a common Validate Credentials API [WIP] Add JSON-Schema create-params and a common Validate Credentials API Aug 8, 2019
@agrare agrare added the wip label Aug 8, 2019
@Fryguy
Copy link
Member

Fryguy commented Aug 8, 2019

Liking this approach!

@skateman
Copy link
Member

skateman commented Aug 9, 2019

I like it too, but RSpec doesn't 😕 what about @Hyperkid123?

@Hyperkid123
Copy link

The schema looks good. Is there supposed to be some validation?

@skateman
Copy link
Member

skateman commented Aug 9, 2019

@Hyperkid123 that's coming in a separate PR here, I'll expose it in an API PR soon.

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.

I started playing with this from the API and found out that the DDF schema doesn't really match the structure of the arguments of the validation method. With this approach we should have some logic on the frontend that does the conversion and that's something we already have and we'd like to get rid of.

@martinpovolny
Copy link
Member

I do not think that we need the schema to be the DDF schema. Rather I'd have something simple that we can use to generate the final DDF schema with some whistles and bells.

Wdyt, @skateman, @Hyperkid123?

@agrare agrare force-pushed the add_common_validate_credentials_api branch from 30b05a2 to 2814656 Compare August 9, 2019 12:52
@Hyperkid123
Copy link

Hyperkid123 commented Aug 12, 2019

@martinpovolny i am not really against using something different as long as we use something explicit. I would like to avoid having something ambiguous (like the mozzila schema).

We should probably consider if this is purely for UI purposes or is there any other reason why would want this? If there is i would consider thinking about something else. Otherwise i don't see a reason why not to use ddf.

@agrare agrare changed the title [WIP] Add JSON-Schema create-params and a common Validate Credentials API Add JSON-Schema create-params and a common Validate Credentials API Aug 15, 2019
@agrare
Copy link
Member Author

agrare commented Aug 15, 2019

@Hyperkid123 how to do optional parameters in DDF? I'd like to add proxy-uri and assume-role but they're not required

@miq-bot miq-bot removed the wip label Aug 15, 2019
@Hyperkid123
Copy link

Hyperkid123 commented Aug 15, 2019

@agrare Do you mean optional in a way that the fields are there but they user does not have to fill them or if someone chooses some option from select for instance and then another field appears?

If you mean the first case: By default all fields are optional (as in they are not required). If you wan to add required validation you must add validator and additional properties for the component to add visual representation (for required the *).

example:

{
  component: 'text-field',
  name: 'field',
  label: 'Validated field!',
  isRequired: true,
  validate: [
    {
      type: 'required-validator',
    }
  ]
}

http://data-driven-forms.surge.sh/renderer/validators#requiredvalidator

@agrare
Copy link
Member Author

agrare commented Aug 15, 2019

Oh okay awesome thanks @Hyperkid123

@agrare agrare force-pushed the add_common_validate_credentials_api branch 3 times, most recently from 08831ba to 43d34bb Compare August 15, 2019 17:22
@Fryguy
Copy link
Member

Fryguy commented Aug 26, 2019

@agrare Should we validate the incoming parameters against the DDF schema?

@agrare
Copy link
Member Author

agrare commented Aug 26, 2019

Should we validate the incoming parameters against the DDF schema?

That'd be great, @Hyperkid123 is there a helper utility to check the data-types that are passed in?

@Hyperkid123
Copy link

@agrare yes. Currently we have just a limited amount of data types but we can add some if we need: http://data-driven-forms.surge.sh/renderer/data-types

What data-types are we talking about? Or do you want to just check the if the string is URL for instance? For that we have a number of different validators we can use.

@agrare agrare force-pushed the add_common_validate_credentials_api branch from 25501a1 to a0316ef Compare August 27, 2019 12:26
@agrare
Copy link
Member Author

agrare commented Aug 27, 2019

What data-types are we talking about? Or do you want to just check the if the string is URL for instance? For that we have a number of different validators we can use.

@Hyperkid123 most likely just that a parameter is a string, is a url string, or is an integer (port).

@agrare
Copy link
Member Author

agrare commented Aug 27, 2019

@Fryguy you want to do ddf validation that in a follow-up?

@Hyperkid123
Copy link

Hyperkid123 commented Aug 27, 2019

@agrare All values are strings by default. There is no data-type URL in JS, but we can add url validator: http://data-driven-forms.surge.sh/renderer/validators#urlvalidators

There is a lot of params for URL validator like port, host, if ip address is valid, etc. Let me know what url format you want and we can add it.

@Fryguy
Copy link
Member

Fryguy commented Sep 4, 2019

Regarding the validation it would be really sweet if we had a DFF -> Hash validation class on the Ruby side. Unfortunately, you'd need a Ruby implementation of each validator, but this would get us server-side validation (to help with direct-api calls) in addition to the client-side validation.

@miq-bot
Copy link
Member

miq-bot commented Sep 4, 2019

Checked commits agrare/manageiq-providers-amazon@7bb21a2~...1127b71 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

@gtanzillo gtanzillo added this to the Sprint 120 Ending Sep 16, 2019 milestone Sep 6, 2019
@gtanzillo gtanzillo merged commit 66107aa into ManageIQ:master Sep 6, 2019
@agrare agrare deleted the add_common_validate_credentials_api branch September 6, 2019 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants