-
Notifications
You must be signed in to change notification settings - Fork 70
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
Authdriver #236
Authdriver #236
Conversation
0d659ec
to
9f0e26c
Compare
linchpin/__init__.py
Outdated
@@ -60,12 +60,15 @@ def get_command(self, ctx, name): | |||
@click.option('-w', '--workspace', type=click.Path(), envvar='WORKSPACE', | |||
help='Use the specified workspace if the familiar Jenkins $WORKSPACE environment variable ' | |||
'is not set') | |||
@click.option('-c', '--creds', type=click.Path(), envvar='LP_CREDS', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the -c
option is already used for config. Just use --creds-path
option here.
linchpin/__init__.py
Outdated
@click.option('-v', '--verbose', is_flag=True, default=False, | ||
help='Enable verbose output') | ||
@click.option('--version', is_flag=True, | ||
help='Prints the version and exits') | ||
@pass_context | ||
def runcli(ctx, config, workspace, verbose, version): | ||
def runcli(ctx, config, workspace, creds, verbose, version): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When adding new args, please add them at the end as a keyword argument. This is especially important for api functionality. In this case, it's unlikely to be a kwarg, but creds_path
should be at the end.
linchpin/linchpin.conf
Outdated
@@ -36,6 +36,7 @@ schemas_folder = schemas | |||
|
|||
resources_folder = resources | |||
inventories_folder = inventories | |||
credentials_folder = credentials |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? I don't see where it's currently used.
…ll string Current built in default jinja filter helps the user to omit/provide default to keys in the ansible module. However , it doesnt provide a way for user to conditionally provide a default value to ie., based on the value provided by the output of previous filter . In order to facilitate the same provide_default filter is being written.
auth_driver module fetches and parses file returns to credentials in auth_var variable making creds accessible inside ansible playbooks.
…it as evar to run_cli
9f0e26c
to
b5e1291
Compare
PR is broken after rebase, will be fixing it soon . |
6e72cad
to
9679c50
Compare
linchpin up on openstack
linchpin destroy on openstack
|
linchpin/api/__init__.py
Outdated
@@ -188,6 +188,8 @@ def run_playbook(self, pinfile, targets='all', playbook='up'): | |||
if self.ctx.cfgs.get('ansible'): | |||
ansible_console = ast.literal_eval(self.ctx.cfgs['ansible'].get('console', 'False')) | |||
|
|||
self.ctx.evars['creds_path'] = self.ctx.creds_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use self.set_evar here. Failed CI due to the value not being there. set_evar makes sure to create the structure for you if it isn't there.
linchpin/api/__init__.py
Outdated
@@ -188,7 +188,7 @@ def run_playbook(self, pinfile, targets='all', playbook='up'): | |||
if self.ctx.cfgs.get('ansible'): | |||
ansible_console = ast.literal_eval(self.ctx.cfgs['ansible'].get('console', 'False')) | |||
|
|||
self.ctx.evars['creds_path'] = self.ctx.creds_path | |||
self.set_evar('creds_path', self.ctx.creds_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not help. self.ctx.creds_path is being set into the same place. Why is this necessary?
d62a6f4
to
afa01b3
Compare
The auth_var is now used across all the playbooks to refer the aws_access_id and aws_secret_key
afa01b3
to
80655fb
Compare
…ssible to unset a var in ansible
97e9309
to
e9cffcf
Compare
4867d2f
to
ee7fd2f
Compare
linchpin up aws # with env vars
linchpin destroy aws
|
linchpin up aws # with --creds-path
linchpin destroy # with --creds-path
|
linchpin up gcloud # with creds-path
linchpin destroy gcloud # with creds-path
|
Note: All the above tests are performed on schema_v4 topologies. As credentials attribute is introduced in schema_v4 only . |
What is the likelihood of removing schema_v3 with this update? Probably shouldn't be maintaining two schemas as that's not a viable option. |
@herlo I also feel we should be using schema_v4 only for auth_driver because it makes no sense in supporting both the schemas from 1.0 release. However , the only consequence is to update all the examples and documentation for openstack , gcloud and aws topologies. |
@samvarankashyap since we agree on this point, please create an issue to remove schema_v3 as part of a 1.1.0 release moving forward. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Summary:
Designed according to PR #76 discussion
Updated schema v4
Updated playbooks
example topology :
cli usage:
or
Issue fixes:
fixes #76
Note:
** should be merged after #218 **
** Needs Rebase **
** Needs testing (for gcloud) **