-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Replacing enable_docker_bridge with a generic option called bootstrap_extra_args #320
Replacing enable_docker_bridge with a generic option called bootstrap_extra_args #320
Conversation
@@ -4,7 +4,7 @@ | |||
${pre_userdata} | |||
|
|||
# Bootstrap and join the cluster | |||
/etc/eks/bootstrap.sh --b64-cluster-ca '${cluster_auth_base64}' --apiserver-endpoint '${endpoint}' --enable-docker-bridge '${enable_docker_bridge}' --kubelet-extra-args '${kubelet_extra_args}' '${cluster_name}' | |||
/etc/eks/bootstrap.sh --b64-cluster-ca '${cluster_auth_base64}' --apiserver-endpoint '${endpoint}' ${bootstrap_extra_args} --kubelet-extra-args '${kubelet_extra_args}' '${cluster_name}' |
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.
Not sure about the no quoting 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.
A section should be added to the readme that talks about the enable_docker_bridge flag since it now doesn't have a variable.
@RothAndrew OK good idea. I've highlighted it in the changelog in the same way it's been done previously |
I was referring more to a section that says something like: "Use the |
OK but where do we put that though? Not in the main readme as that is documentation for the module itself, not for random gotchas and issues. We could create another file in |
@RothAndrew I will merge this as I've tested it both with and without the docker bridge option and the nodes join the cluster. If you feel there is some documentation about that issue that is missing then you can make a PR for that 🙂 |
@max-rocket-internet sorry, this got pushed off my radar. There could definitely be a troubleshooting or FAQ doc, that's a good idea. On a more troubling topic, this should have resulted in a major version bump, since it has a breaking change. Recommend immediately releasing a v2.3.2 tag with this change reverted and then releasing v3.0.0. Without doing that, someone who had the version they are using as Edit: Here's the docs page for module versions. |
I think you are right but it's a little late now. I don't want to delete a tag as that is just messy.
Not exactly. The docker bridge option will stop working ONLY if they update their version of this module without checking what's changed AND if they don't look at what terraform is actually changing. |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Description
Replacing enable_docker_bridge with a generic option called bootstrap_extra_args
Fixes #310 #317
Checklist
terraform fmt
andterraform validate
both work from the root andexamples/eks_test_fixture
directories (look in CI for an example)