-
Notifications
You must be signed in to change notification settings - Fork 265
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
Update nginx example to work #625
Conversation
c6d80c9
to
c712ada
Compare
76082bf
to
a79f2a5
Compare
@andrewsomething @scotchneat Let me know what more is needed to get this merged. |
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 change is great @atombrella!
This isn't a blocker but since the release of Terraform 0.12.14, Interpolation-only expressions are deprecated
. This applies to the ${xxx}
interpolation syntax within quotes but without any other text within the quotes.
Although it's still a warning, I think we could take this opportunity to address it. It can be resolved by removing the quotes. I included some suggestions inline but didn't cover all the instances. You can check if you still get the warning (as long as you're using terraform 0.12.14 or newer - I suggest >= 0.14) by running terraform plan
on this example locally.
@scotchneat Thanks :) The PR has been updated. My experience with Terraform is a bit limited, but I have used Please adjust the commit message if needed. I couldn't really see a pattern for changes of this type. |
Co-Authored-By: Cesar Garza <[email protected]>
@scotchneat No warnings with tflint :) I won't make anymore changes to this PR. |
Added a cloud-init file and passed it into to the user_data attribute of the droplet to avoid apt-get lock issue.
Looks great! I added a minor change to fix a lock issue I ran into. It's the same problem #595 addresses. |
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'd love to merge the change as is since it covers the original purpose of the change.
However, there's one more change that would allow this example to run from a clean clone.
Currently, the ssh key is hardcoded to a specific key id. This will fail until the user updates the key. I've added some small suggestions inline that will make this dynamic and should allow users to run the example straight away.
Co-authored-by: Cesar Garza <[email protected]>
Co-authored-by: Cesar Garza <[email protected]>
Co-authored-by: Cesar Garza <[email protected]>
OK. I was about to ask about this as well. Thanks! :) |
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! Thank you so much!
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! Thanks again for the contribution
Hey, @atombrella - Thanks a ton for this sweet PR! 😄 Would you please shoot me an email when you get a chance? |
I couldn't see CoreOS in the list under the droplets anymore. Because the droplet is created with IPv6, I'd like to encourage people to support it better with a AAAA DNS record.
Please see #622