Skip to content
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

Fix documentation (broken partial rendering and missing id) #4862

Merged
merged 3 commits into from
Oct 29, 2018

Conversation

sportebois
Copy link
Contributor

As I used the service registration doc I stumbled upon a few errors, which I try to address here:

Broken partial rendering for service (de)registration (CLI)

Problematic pages:

Both pages include a partial for the API options, but the current version doesn't render this partial:
Current live website capture:
image

The fix is simple (but took me a while since I'm new to middleman), and it really comes down to renaming those files so that the partials are evaluated and not just outputted as raw content.

Here's a sample of the local test as I verified the fix:
image

Incomplete JSON sample for the service registration

Page: https://www.consul.io/docs/agent/services.html
Two small issues here:

  • JSON is invalid, we there's a missing comma before the connect block (nothing a user wouldn't be able to fix after copying the template, but I believe having a template ready to be customized rather than fixed is nice)

the other point is really nitpicking, but the template is preceded by (emphasis mine)

This example shows all possible fields
then just after the template we have
A service definition must include a name and may optionally provide an id, tags, address, meta, port, enable_tag_override, and check. The id is set to the name if not provided. It is required that all services have a unique ID per node, so if names might conflict then unique IDs should be provided.

So the template is supposed to be complete, and the very first field described is the id field, but this field is not present in the template?!
In this PR I added the field to remove any confusion or "oh wait, did I missed something?" moment for the future reader.

Copy link
Member

@hanshasselberg hanshasselberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

@hanshasselberg hanshasselberg merged commit 1823a35 into hashicorp:master Oct 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants