docs: correct what associate_public_ip does, and four smaller README fixes - #64
Merged
Conversation
The option table described associate_public_ip as setting up static NAT. It does not: it associates a public address and creates a port forwarding rule for the one port the configured transport connects on. Someone reading "static NAT" would reasonably expect the whole instance to be reachable through that address, and then find that nothing but SSH or WinRM is. Also drops the duplicate disable_ssl_validation row, which was documented twice with two different descriptions; states cloudstack_sync_time's default as unset rather than 0, since the driver has no default_config for it and skips the wait entirely; documents kitchen doctor, which this driver implements and the README never mentioned; and removes the ask to port the driver off SSHBase, which was done in 1.0.0 and which the README's own troubleshooting section already says was done. Signed-off-by: Tim Smith <tim@mondoo.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I went through every option in the README against
default_configand everyplace the driver reads
config[...]. The good news first: there are no optionsin the code that the README leaves out, and no options in the README that the
code no longer reads. The defaults are right too, apart from one noted below.
What I did find:
associate_public_ipis documented backwardsThe table said:
It does not set up static NAT.
Networking#associate_public_ipcallsassociateIpAddress, andNetworking#create_port_forwardthen callscreatePortForwardingRulewithprivateportandpublicportboth set to theone port the configured transport connects on. Static NAT
(
enableStaticNat) makes the whole instance reachable through the address; aport forwarding rule makes exactly one port reachable.
That is a meaningful difference for anyone whose test needs to reach a service
on the instance — they will read "static NAT", expect port 8080 to work, and
find that only SSH does.
cloudstack_create_firewall_rulehas the sameone-port scope, and the Windows section further down already describes the
behaviour correctly, so the table row was the outlier.
I have corrected the row and added a note to the advanced-networking example
pointing at
cloudstack_vm_public_ipfor people who want static NAT.disable_ssl_validationwas documented twiceOnce under Credentials and once under Other, with two different descriptions.
Kept the Credentials one.
cloudstack_sync_timeclaimed a default of0There is no
default_configfor it. When it is unset the driver skips the waitentirely rather than sleeping zero seconds. Same outcome, but "unset" is what
is actually true, and it does not imply a default that is not there.
kitchen doctorwas undocumentedThe driver implements
doctor, and it is genuinely worth knowing about: thisdriver declares no
required_config, so a missing endpoint or credential isnot caught at load time and instead fails part way through a deploy.
doctornames every unset setting in one run and then makes a real API call to check
the keys. Added a short section for it.
The contributing section asked for work that is already done
That was done in 1.0.0 — the README's own troubleshooting entry tells people on
0.24.0 to upgrade for exactly that reason. Pointed at CONTRIBUTING.md's actual
list instead.
Testing
markdownlint-cli2clean, which is what CI runs on**/*.md.Conflicts
#62 touches one line of this README (the
fog-cloudstackbullet underRequirements). Nothing here goes near it.