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 vault_password_file param to PlaybookRunner #129

Merged

Conversation

jan-zmeskal
Copy link

Adding vault_password_file parameter to PlaybookRunner.run() method. This is needed if we want to use Ansible vault protected variables files.

@codecov
Copy link

codecov bot commented May 7, 2020

Codecov Report

Merging #129 into master will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #129      +/-   ##
==========================================
+ Coverage   79.78%   79.82%   +0.04%     
==========================================
  Files          19       19              
  Lines        1865     1869       +4     
==========================================
+ Hits         1488     1492       +4     
  Misses        377      377              
Impacted Files Coverage Δ
rrmngmnt/playbook_runner.py 97.46% <100.00%> (+0.06%) ⬆️
rrmngmnt/common.py 95.12% <0.00%> (+0.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c43968b...7672cca. Read the comment docs.

self._upload_file(vault_password_file)
)
)

Copy link
Member

@lukas-bednar lukas-bednar May 11, 2020

Choose a reason for hiding this comment

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

on line 188, I guess that you will have to upload that key file to host server, in case upload_playbook=True

EDIT:

Actually other way around. You have to pass local path to key in case upload_paybook=False.

Copy link
Author

Choose a reason for hiding this comment

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

Well, you always have to pass local paths to both vault_password_file and to vars_files. No matter the value of upload_playbook. Let me update the docstring so this is clearer.

Copy link
Member

Choose a reason for hiding this comment

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

self.upload_file returns path on remote host. and in case you pass upload_playbook=False you should pass "--vault-password-file={}".format(vault_password_file) , no?

Copy link
Author

Choose a reason for hiding this comment

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

Well, you do have a point, but exactly the same also goes for vars_files. When I originally wrote this class and method, I never intended for user to upload any of those files manually. The parameters upload_playbook was implemented by @santos1709 and back then it was agreed that we wouldn't accommodate vars_files to it. However, it we want vars_files and vault_password_file to react on the value of upload_playbook, I think this requires bigger and more careful change. If that's the case, I suggest tracking it in a new issue/ticket.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, please open an issue. and we will merge this one.

Copy link
Author

Choose a reason for hiding this comment

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

Tracked in #130 . I'll also create a ticket in our Jira board

@lukas-bednar lukas-bednar added this to the 0.1.24 milestone May 12, 2020
@lukas-bednar lukas-bednar merged commit 9a6a787 into rhevm-qe-automation:master May 12, 2020
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.

2 participants