-
Notifications
You must be signed in to change notification settings - Fork 5
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
Improvement/dry provisioning #86
Conversation
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 all looks good to me. I tested it in my local workspace and it seems to work as advertised, but I didn't try it in Production yet.
Hold the phone- I think I found a bug. Checking now...
There is a bug, but it's not in your code. However, this seems like a good place to fix it. The problem is in these four places:
In all of the above, the code references
Unfortunately, when I make that change, I run into this open Terraform issue: Example: In my local workspace, I have zero running nmap instances and I want to ramp up to three of them. The existing code has no problem with this because every
Anybody have any ideas on how to fix this? |
The bug report you referenced says:
Did you try that? |
I missed that- trying it now. |
That |
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.
Please consider my comments before merging.
def main(): | ||
# get workspace | ||
workspace = get_terraform_workspace() | ||
print('Current Terraform workspace = ', workspace) |
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.
What about using logging
instead of print
statements?
terraform/configure.py
Outdated
WORKSPACE_CONFIGS constant below. | ||
''' | ||
|
||
import sys |
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 imports to be sorted in three groups in this order:
- Python built-ins
- External dependencies
- Local dependencies
Then, within each group, I like the imports to be sorted alphabetically. This makes it easy to find imports as the list grows in length.
What do you think about doing that here?
terraform/configure.py
Outdated
|
||
Due to a terraform limitation, modules can not be scaled with the "count" | ||
keyword the same way resource can. This leads to a great deal of copying and | ||
pasting in order to get provisioners and other modules to execute correctlyself. |
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.
Remove correctlyself
and replace with correctly
. 😃
…st nmap/nessus instance; see #86 (comment) Update nmap and nessus dynamic templates to work correctly after the change above
…yhy_amis into improvement/dry_provisioning
…yhy_amis into improvement/dry_provisioning
@jsf9k , the Example: When I attempt to scale up from 1 to 2 nmap instances, Terraform wants to recreate
It is the same issue as mentioned in this comment on Terraform issue 14536. We need to figure out how to get Terraform to avoid destroying the existing data attachment(s) in this scenario. |
…e parameters, but use element() syntax to access correct instance id in destroy provisioners. This avoids the situation where scaling up nmap/nessus instances would result in the destruction of existing volume attachments, as described in #86 (comment).
My last commit fixes the issue I described here. |
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 now gets my official 👍
Dynamic provisioning in the house. This works around this issue: hashicorp/terraform#953