-
Notifications
You must be signed in to change notification settings - Fork 35
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
New module plan_stash #113
New module plan_stash #113
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## stable-plan-stash #113 +/- ##
====================================================
Coverage ? 74.10%
====================================================
Files ? 17
Lines ? 1058
Branches ? 186
====================================================
Hits ? 784
Misses ? 243
Partials ? 31
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
8695ad8
to
fea7b57
Compare
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 5m 12s |
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 3m 14s |
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.
See one comment about the action plugin, but generally this looks good and works as expected.
…_argument_spec() function
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 4m 04s |
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.
👍
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.
Using this, I'm realizing we may want to make this module work both for stashing and for loading the plan file. The only way to safely write the b64 encoded data to a zip file would be for the user to have a shell task that calls out to base64, which is kind of ugly.
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.
@gravesm it makes sense to support the load of the plan file, but currently, a shell task is not the only way to do that, we can do it using ansible.builtin.copy
module
- copy:
dest: /path/to/plan/file
content: "{{ terraform_plan | b64decode }}"
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.
It's unclear to me that this is always going to work, though. The documentation for the b64decode filter pretty clearly says this is likely to corrupt the binary data.
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.
ok I am going to update the module to work like this
Stash the terraform plan file
- cloud.terraform.plan_stash:
mode: stash
path: terraform.tfplan
var_name: terraform_plan
Load the terraform plan file
- cloud.terraform.plan_stash
mode: load
path: terraform.tfplan
var_name: terraform_plan
WDYT ? @gravesm @hakbailey
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.
Yeah, this is along the lines of what I'm thinking. My only suggestion would be to use state
instead of mode
.
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 3m 50s |
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 5m 53s |
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.
I like the addition of the load option! See some minor comments in the documentation block. I also wonder if we want to note something about how set_stats
does not set variables in the way set_facts
does during local ansible-playbook runs...basically explaining that this module is intended for use within an AAP workflow.
3a876f5
to
a2d53e2
Compare
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 4m 47s |
@@ -29,7 +29,7 @@ commands = | |||
|
|||
[testenv:black] | |||
deps = | |||
black | |||
black==23.12.1 |
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.
Why should it be tied to this specific version of black?
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.
because a new release of black may generate formatting errors.
9495909
to
2689553
Compare
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 3m 29s |
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 7m 15s |
Backport to stable-2: 💚 backport PR created✅ Backport PR branch: Backported as #127 🤖 @patchback |
* Using an action plugin * add integration tests for the plan_stash module * Update default plan stash variable to terraform_plan * Update action module with ansible arg specs validation using validate_argument_spec() function * add note to suggest use of no_log: true * Add new feature allowing to load the terraform file * Fix sanity tests * minor refactoring (cherry picked from commit c70ff08)
* Using an action plugin * add integration tests for the plan_stash module * Update default plan stash variable to terraform_plan * Update action module with ansible arg specs validation using validate_argument_spec() function * add note to suggest use of no_log: true * Add new feature allowing to load the terraform file * Fix sanity tests * minor refactoring (cherry picked from commit c70ff08)
[PR #113/c70ff085 backport][stable-2] New module plan_stash This is a backport of PR #113 as merged into main (c70ff08). SUMMARY A module to perform base64-encoding and set_stats for a terraform plan file. This will be useful to pass artifacts between workflows in AAP ISSUE TYPE New Module Pull Request Reviewed-by: Alina Buzachis Reviewed-by: Mike Graves <[email protected]>
SUMMARY
A module to perform base64-encoding and set_stats for a terraform plan file. This will be useful to pass artifacts between workflows in AAP
ISSUE TYPE