-
Notifications
You must be signed in to change notification settings - Fork 309
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: update quick start documentation #956
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: NotAwar The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @NotAwar. Thanks for your PR. I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/assign @jmhbnz |
need okay from owners so: requesting review from the only people who have committed to this file previously: |
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.
Hi @NotAwar,
Thanks for this. I've gone through and made some line-by-line comments , which I'll leave here for reference because some of the comments are generally applicable, but the more significant issue is that I'm not sure we need this change.
The main shift in this PR is breaking out the developer workflow from the operator workflow. I don't think this page needs that complexity, and the operator workflow would be so similar (at least as presented here) that it shouldn't be required for someone looking to set up etcd quickly for an evaluation (this page's audience is a decision maker who wouldn't necessarily need to know all the ins and outs of an etcd installation for production).
content/en/docs/v3.5/quickstart.md
Outdated
``` | ||
|
||
3. **Configure and Secure etcd** | ||
Check out the Operator Guide for steps on clustering, securing with TLS, and performance tuning. |
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.
Is there a link we could provide 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.
I added some links checkout the latest changes to this section
content/en/docs/v3.5/quickstart.md
Outdated
1. **etcd Installed** | ||
Build or download etcd from [Install][] and verify that the `etcd` binary is in your system’s PATH. | ||
{{% alert color="warning" %}}**Important**: Ensure that you perform the last | ||
step of the installation instructions to verify that `etcd` is in your path. | ||
{{% /alert %}} |
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.
do we also need to prompt folks to install etcdctl separately? (I needed to on my test setup)
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'm not sure we need an alert here referring to the content of another page. We can trust that folks will follow the instructions there.
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.
do we also need to prompt folks to install etcdctl separately? (I needed to on my test setup)
I think this depends on your install method, if you build from the binaries you might have to prompt for etcdctl separately. I did a clean "brew install etcd", after which both "etcd" and "etcdctl" were added to my path and were viable commands.
For command "etcd --version":
etcd Version: 3.5.18
Git SHA: 5bca08ec1
Go Version: go1.23.5
Go OS/Arch: darwin/arm64
For command "etcdctl version":
etcdctl version: 3.5.18
API version: 3.5
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.
removed the alert, diff
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, let's leave it as is then (no need for an extra prompt), we can leave the details of various ways of installing to the installation page.
content/en/docs/v3.5/quickstart.md
Outdated
```console | ||
$ etcd | ||
{"level":"info","ts":"2021-09-17T09:19:32.783-0400","caller":"etcdmain/etcd.go:72","msg":... } | ||
⋮ | ||
``` | ||
|
||
{{% alert color="info" %}}**Note**: The output produced by `etcd` are | ||
[logs](../op-guide/configuration/#logging) — info-level logs can | ||
be ignored. {{% /alert %}} | ||
|
||
2. **Interact with etcd** | ||
In a separate terminal, run the following commands to set and retrieve a key: | ||
|
||
```console | ||
$ etcdctl put greeting "Hello, etcd" | ||
OK | ||
``` | ||
|
||
```console | ||
$ etcdctl get greeting | ||
greeting | ||
Hello, etcd | ||
``` |
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 tested this locally using lima with an ubuntu image, these instructions work.
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.
Ty for confirmation! 😄
content/en/docs/v3.5/quickstart.md
Outdated
Continue with the [Developer guide][], which includes the [Set up a local cluster][] page, [API][] references, and integration examples. | ||
|
||
- #### Explore the gRPC [API][] | ||
|
||
- #### Find [language bindings and tools][integrations] | ||
|
||
- ### **Operations:** | ||
|
||
Proceed to the [Operations guide][] for advanced configuration, clustering, security, and performance tuning. | ||
|
||
- #### Set up a [multi-machine cluster][clustering] | ||
|
||
- #### Use TLS to [secure an etcd cluster][security] | ||
|
||
- #### [Tune etcd][tuning] | ||
|
||
- #### Learn how to [configure][] etcd |
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 section feels incomplete and doesn't seem to render correctly. Please take a look a the deploy preview.
Also, i recommend against using links in headings. See https://kubernetes.io/docs/contribute/style/style-guide/#headings.

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 was kind of unsure as to how this would render and also how the content should be displayed, I changed it, though as has been mentioned in #782 I believe this section should probably just point to "Getting Started" for the dev-guide and op-guide
I've struck some of my initial comments based on the issue that this PR is based on. |
@NotAwar , The single-node install is suitable for development/test/evaluation only. This change makes that clear, but I think @nate-double-u 's point (correct me if I'm wrong, Nate) is that a Quick Start page might not be the best place to describe the production server install. An alternative that might satisfy all the concerns here: make it clear on the Quick Start page that the one-node quick install is appropriate for dev/test/eval, and instead of including the "full" install in the Quick Start, point admin/production users to the full install page. Again, the point of my suggestion is to clearly explain that these are two different install procedures with two vastly different use cases. This might be understood by most non-novice users, but it's not spelled out in the documentation. It doesn't take much effort to avoid confusion and guide users to the appropriate procedure for their use case. |
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.
Thanks @NotAwar.
@dwelsch-esi and @nate-double-u - I think that we need to clarify the guidance / suggestions of #782 first before making progress here. See #782 (comment).
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.
GH quickstart template also uses one word.
@NotAwar - also please sign all commits in this PR per DCO requirements. |
@NotAwar, in the checks box you can find the DCO check -- it doesn't look like one, but it's a link that goes to a page that will show you how to solve the DCO issue. ![]() |
Signed-off-by: Awar Abdulkarim <[email protected]>
Signed-off-by: Awar Abdulkarim <[email protected]>
Signed-off-by: Awar Abdulkarim <[email protected]>
Bumps [hugo-extended](https://github.com/jakejarvis/hugo-extended) from 0.143.1 to 0.144.2. - [Commits](jakejarvis/hugo-extended@v0.143.1...v0.144.2) --- updated-dependencies: - dependency-name: hugo-extended dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: Awar Abdulkarim <[email protected]>
Bumps [netlify-cli](https://github.com/netlify/cli) from 18.1.0 to 19.0.0. - [Release notes](https://github.com/netlify/cli/releases) - [Changelog](https://github.com/netlify/cli/blob/main/CHANGELOG.md) - [Commits](netlify/cli@v18.1.0...v19.0.0) --- updated-dependencies: - dependency-name: netlify-cli dependency-type: direct:development update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: Awar Abdulkarim <[email protected]>
Could have probably done this more cleanly, but ty very much @nate-double-u and @chalin |
Signed-off-by: Awar Abdulkarim <[email protected]>
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 updates to versions need to go in a separate PR.
Overall: I can see what you're doing with reorganizing quickstart, and it makes sense. However, this needs to go on a development branch, because we can't merge it into main until all supporting documents are written. Would you like me to create a dev branch?
@@ -19,9 +19,9 @@ | |||
"devDependencies": { | |||
"autoprefixer": "^10.4.20", | |||
"docsy": "github:google/docsy#semver:0.11.0", | |||
"hugo-extended": "0.143.1", | |||
"hugo-extended": "0.144.2", |
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.
Please do not roll version upgrades together with content changes. This should be a separate PR.
"mkdirp": "^3.0.1", | ||
"netlify-cli": "^18.0.1", | ||
"netlify-cli": "^19.0.0", |
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.
Please do not roll version upgrades together with content changes. This should be a separate PR.
@dwelsch-esi @nate-double-u and now @jberkus - my concerns still stand: IMHO we need to discuss the issue guidance before trying to resolve it. It is counter productive to do while working through a proposal in this PR. How about we meet during the next "CNCF Tech Writers Office Hours" time slot (Wed. March 26 11 am ET)? |
+1, I'll be there :) |
Yes please! Will revert version bumps in this branch. |
Fix update quick start documentation
Description
Updated documentation according to issue #782 , also added more links and split documentation into two paths development and operations. Not sure if this is what @dwelsch-esi is looking for, but hopefully it's at least close!
Related Issue
#782
Preview
Contrast with: