-
Notifications
You must be signed in to change notification settings - Fork 467
Pull request to fix #191 #192
base: master
Are you sure you want to change the base?
Conversation
…documentation fixes
…documentation fixes
…documentation fixes
…documentation fixes
…documentation fixes
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 is great work, thank you 👍
I'll kick off tests shortly.
Hm, many of the tests failed:
From what I can see, it's able to build the AMI and run
It basically is looking for a 501 over and over again and never getting it. Could you run one of these examples and see what the issue is? |
These tests are pretty slick. I think I've got things set up to run them locally now. I'll give it a shot and report my findings. |
@brikis98 I was able to recreate these errors with my branch, but not being able to connect any dots, I tried to run the entire suite against the master branch I forked -- i.e. without my commits -- and got the same errors. I think some of them are failing because I don't have autoseal keys setup, but I'm definitely seeing the 501 error in there as well. Another interesting thing is if I pick one of the failing tests (that don't use autoseal) and target it individually with I don't know if that means there's a race condition in the failing tests or something to do with running all the tests in parallel? Is there a way you can trigger the circleci tests against master and see if I'm looking at things correctly that the issue is upstream from my branch? |
Thanks for taking a look! I'll try to dig in this week to see if things are working on |
There are indeed some issues on |
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, thank you for your patience! We merged fixes in #195. Could you rebase?
This fixes the --agent-ca-cert-file flag leaving handling of the typo in place for backward compatibility.
I also noticed another typo in the documentation
--dynamo--table
that is fixed here as well. This one was just an echo bug, so it didn't seem necessary to try to handle the typo.https://github.com/hashicorp/terraform-aws-vault/blob/master/modules/run-vault/run-vault#L48
I also noticed the README.md was out of date with the new flags, so did my best to update that documentation accordingly.
I don't think there are any unit tests that apply here so I didn't run anything, but I did test the script for backward compatibility in my environment and both versions of the flag worked as expected.