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

[WIP] Become plugins #50421

Open
wants to merge 9 commits into
base: devel
from

Conversation

Projects
None yet
7 participants
@abadger
Copy link
Member

abadger commented Dec 31, 2018

SUMMARY

become_plugins using the CLIARGS context so we don't have to pass options around everywhere

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

become plugins

ADDITIONAL INFORMATION

The main thing yet to do is to get rid of play.Play._set_from_CLI_options and base.FieldAttributeBase._set_options. Those should be implemented via FieldAttribute defaults instead.

This is built on top of #50069 which should be merged first.

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Dec 31, 2018

@abadger This PR was evaluated as a potentially problematic PR for the following reasons:

  • More than 50 changed files.

Such PR can only be merged by human. Contact a Core team member to review this PR on IRC: #ansible-devel on irc.freenode.net

click here for bot help

@abadger abadger force-pushed the abadger:become_plugins branch 5 times, most recently from 489a3fa to 2239938 Dec 31, 2018

@abadger abadger referenced this pull request Jan 3, 2019

Closed

Become plugins #38861

quater and others added some commits Oct 24, 2018

[WIP] become plugins
Move from hardcoded method to plugins for ease of use, expansion and overrides
  - load into connection as it is going to be the main consumer
  - play_context will also use to keep backwards compat API
  - ensure shell is used to construct commands when needed
  - migrate settings remove from base config in favor of plugin specific configs
  - cleanup ansible-doc
  - add become plugin docs
  - remove deprecated sudo/su code and keywords
  - adjust become options for cli
  - set plugin options from context
  - ensure config defs are avaialbe before instance
  - refactored getting the shell plugin, fixed tests
     - changed into regex as they were string matching, which does not work with random string generation
     - explicitly set flags for play context tests
 - moved plugin loading up front
 - now loads for basedir also
 - allow pyc/o for non m modules
 - fixes to tests and some plugins
 - migrate to play objects fro play_context
 - simiplify gathering
 -  added utf8 headers
 - moved option setting
 - add fail msg to dzdo
 - use tuple for multiple options on fail/missing
 - fix relative plugin paths
 - shift from play context to play
 - all tasks already inherit this from play directly
 - remove obsolete 'set play'
 - correct environment handling
 - add wrap_exe option to pfexec
 - fix runas to noop
 - fixed setting play context
 - added password configs
 - removed required false
 - remove from doc building till they are ready

future development:
  - deal with 'enable' and 'runas' which are not 'command wrappers' but 'state flags' and currently hardcoded in diff subsystems
cleanup
  remove callers to removed func
  removed --sudo cli doc refs
  remove runas become_exe
  ensure keyerorr on plugin
  also fix backwards compat, missing method is attributeerror, not ansible error
  get remote_user consistently
  ignore missing system_tmpdirs on plugin load
  correct config precedence
  add deprecation
  fix networking imports
  backwards compat for plugins using BECOME_METHODS

@abadger abadger force-pushed the abadger:become_plugins branch from 2239938 to 43b42db Jan 4, 2019

abadger added some commits Dec 31, 2018

Port become_plugins to context.CLIARGS
This is a work in progress:
* Stop passing options around everywhere as we can use context.CLIARGS
  instead
Rename BECOME_PLUGIN_PATH to DEFAULT_BECOME_PLUGIN_PATH
As alikins said, all other plugin paths are named
DEFAULT_plugintype_PLUGIN_PATH.  If we're going to rename these, that
should be done all at one time rather than piecemeal.
Stop loading values from the cli in more than one place
Both play and play_context were saving default values from the cli
arguments directly.  This changes things so that the default values are
loaded into the play and then play_context takes them from there.

@abadger abadger force-pushed the abadger:become_plugins branch from 43b42db to 70d576e Jan 4, 2019

class BecomeModule(BecomeBase):

name = 'pmrun'
prompt = 'Enter UPM user password:'

This comment has been minimized.

@AlanCoding

AlanCoding Jan 8, 2019

Member

This is very concern to us regarding the subsequent adoption by AWX / Tower. We need these prompts to be machine-parsable. While, yes, you do provide documentation that will probably have the prompt text in ansible-doc, correctly using that to get this info assumes a great deal of things are connected which are very specific to the specific project folder structure, Ansible config, and other things.

You can see our implementation here:

https://github.com/ansible/awx/blob/devel/awx/main/tasks.py#L1354-L1357

We are using the pexpect library to search for the string "PMRUN password:" in the standard out. If that pattern is broken, I don't know how we will deal with it.

This comment has been minimized.

@sivel

sivel Jan 8, 2019

Member

I believe this is the prompt that we expect pmrun to prompt us with, not what we will prompt the user with:

https://github.com/ansible/ansible/pull/50421/files#diff-6ce98545d6c25c12c5747868c8866e6aR61

This comment has been minimized.

@jbradberry

jbradberry Jan 8, 2019

Contributor

Tagging myself in since I'll be working on the AWX support...

This comment has been minimized.

@AlanCoding

AlanCoding Jan 8, 2019

Member

Ah, I see, that's great if that's the case. I think we just need to test this out.

One to throw away
This is a set of hacks to get setting FieldAttribute defaults to command
line args to work.  It's not fully done yet.

After talking it over with sivel and jimi-c this should be done by
fixing FieldAttributeBase and _get_parent_attribute() calls to do the
right thing when there is a non-None default.

What we want to be able to do ideally is something like this:

class Base(FieldAttributeBase):
    _check_mode = FieldAttribute([..] default=lambda: context.CLIARGS['check'])

class Play(Base):
    # lambda so that we have a chance to parse the command line args
    # before we get here.  In the future we might be able to restructure
    # this so that the cli parsing code runs before these classes are
    # defined.

class Task(Base):
    pass

And still have a playbook like this function:

---
- hosts:
  tasks:
  - command: whoami
    check_mode: True

(The check_mode test that is added as a separate commit in this PR will
let you test variations on this case).

There's a few separate reasons that the code doesn't let us do this or
a non-ugly workaround for this as written right now.  The fix that
jimi-c, sivel, and I talked about may let us do this or it may still
require a workaround (but less ugly) (having one class that has the
FieldAttributes with default values and one class that inherits from
that but just overrides the FieldAttributes which now have defaults)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment