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

Is there documented good way a plugin should deal with params ? #6

Open
fkuep opened this issue Mar 30, 2021 · 6 comments
Open

Is there documented good way a plugin should deal with params ? #6

fkuep opened this issue Mar 30, 2021 · 6 comments

Comments

@fkuep
Copy link
Owner

fkuep commented Mar 30, 2021

Now if go ahead and model "url is set " into this diagram in Readme

I still have three problems:

  • I don't understand about ini and env values and their precedence.
  • Should we even (as think now) most in the constructor vs the kv.get request (in case consul decides tokens have to be inheaders) ?
  • Is there a documented best practise or a does a mock unit test make sense ?
  • Also there is terms in which I suppose with_module_ -multi-invocation is handled..
@fkuep
Copy link
Owner Author

fkuep commented Mar 31, 2021

"Prior art" to this ticket: ansible/proposals#15
This lead to a comment in LookupBase

@bcoca
Copy link

bcoca commented Apr 29, 2021

I just ported all core lookups that use options to the plugin config system, this should be the standard way of handling options.

The current docs are generic about plugins but I'll follow up with specifics per plugin type, including lookups, but the short of it is:
a) create DOCUMENTATION with the options you want and the various configuration methods available. Declaring the option makes it 'directly settable' in lookup via k=v argument passing lookup('x', <terms>. optionname='value')
b) call set_options() early in your run method
c) use get_option() to get the 'resolved' value you want.

draft ansible/ansible#74495 (feedback welcome)

@fkuep
Copy link
Owner Author

fkuep commented Apr 30, 2021

I just ported all core lookups that use option
..
current docs are generic about plugins but I'll follow up with specifics per plugin type
..
draft ansible/ansible#74495 (feedback welcome)

@bcoca Thank You veriy much for the effort!
That makes it much easier for a beginner like me to know how to start developing!

  • Now I fell more confident in my understanding , that where possible I should not try and fiddle parsing terms myself for options.
  • Is there a way (argspec ?) to tell a user that an option does not exsist in a lookup plugin ?
    • or validate options in a standard way ?

You probably saw, that I am trying to deal with legacy code:
ansible-collections/community.general#2126 (comment)

In #7 You can see me discovering (after days of chewing on the code) that:

  • certain options must be put inside terms
  • others must be put in oprtion= format

And if put inside terms, there is some validation, where I can see an error, when I use a wrong option.
Whereas set_options leads to superfluous options being ignored silently.

So knowing "the way" to input validation for set_option in lookup plugins would greatly help me over that hurdle.

@bcoca
Copy link

bcoca commented Jun 2, 2021

Is there a way (argspec ?) to tell a user that an option does not exsist in a lookup plugin ?

set_options/set_option does that validation, the DOCUMENTATION is your argspec

certain options must be put inside terms

This should be discontinued, but if you look at core lookups, I kept this for backwards compatibility (in next version I plan to add deprecation message) see ansible/ansible#74108 (PR that migrated all of them to use set_options/get_option).

You can parse this way and use set_option to override the settings for that term (you might want to reset to global at start of term loop).

others must be put in oprtion= format

This should be the standard going forward

@fkuep
Copy link
Owner Author

fkuep commented Jun 2, 2021

Is there a way (argspec ?) to tell a user that an option does not exsist in a lookup plugin ?

set_options/set_option does that validation, the DOCUMENTATION is your argspec

Well then, for example... :
What should be written into the DOCUMENTATION
in order to abort execution or display a warning, if a user sends the non-exsiting option 'schema' instead of 'scheme' ?

The legacy code does this while parsing the "terms for options".

@bcoca
Copy link

bcoca commented Jun 2, 2021

use set_optoins/set_option, they do all the validation on required, type and matching existing settings.

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

No branches or pull requests

2 participants