-
Notifications
You must be signed in to change notification settings - Fork 330
Add Prereq section and rewrite Quickstart for local etcd setup #981
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
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ronaldngounou 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 @ronaldngounou. 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. |
/ok-to-test |
@spzala |
I'm taking a look at this merged PR to fix the remaining markdown lint. |
c838332
to
0ae2c81
Compare
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.
@ronaldngounou thanks for working on it. It will help if you break down the changes in separate PRs or commits (per task- e.g., pre-req as one change) to help with review.
In the PR description you have mentioned that this PR is for pre-req section for the other issue comment(s) from @chalin but I don't see any pre-req section in your changes. Also, I didn't understand why you have removed link for install (i.e. https://etcd.io/docs/v3.5/install/) and added install instructions. If you provide a small section wide PRs/commits with reasons for making changes, it will help review better. Thanks again!
Thanks for the feedback. @spzala
I replaced the prerequisites section with requirements for a better wording. Would that be okay if I rename Requirements to prerequisites? As @dwelsch-esi mentioned in this post,
I think having a short quickstart page would help the user get up to speed faster. In addition, after this is closed, I will restructure the installation page as @dwelsch-esi suggested.
What can I do now in order to help review better? I will take note of this for my next PR. |
@ronaldngounou I prefer "Prerequisites" because it cues the reader that it's something they need to do before the procedure. I think either will work. Most importantly, I'd advise being consistent throughout the documentation. That is, all the procedures in the documentation should use the same term. |
Alright. I will change that and submit a PR. |
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 @ronaldngounou Please see my inline comments.
content/en/docs/v3.5/quickstart.md
Outdated
Make sure you're on a supported platform before getting started: | ||
|
||
2. Launch `etcd`: | ||
- Operating System: Ubuntu 22.04+ or macOS 11+. Windows users can use WSL2. |
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.
Where did you get these operating systems? This is what we have as supported platform, https://etcd.io/docs/v3.5/op-guide/supported-platform/ I would suggest to provide a link here. Keeping one page for for supported platforms will avoid need for dual maintenance as well.
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.
@spzala I agree that normally maintaining duplicate information isn't worth it. However, the Quick Start guide is a special case. The prereqs here should be only the OSes that can be installed using the quick start procedure. The regular installation guide should be a detailed guide to all installations.
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.
@dwelsch-esi thanks for your thoughts here and other places! I agree and I also like as much as information on the same page (vs links). It's just that if we run into two set of instructions (due to dual maintenance when we forgot to make changes at both places when something change) that doesn't match that will be bad.
- [Supported platforms architecture](/docs/v3.5/op-guide/) | ||
- Tools installed: | ||
- `curl` and `tar` | ||
- Internet connection to download the latest etcd [release](https://github.com/etcd-io/etcd/releases/). |
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 don't think we want to put 'internet connection' as a pre-req. It's implied usually with every tools that need download.
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.
@spzala I agree. Internet is pretty much assumed these days.
content/en/docs/v3.5/quickstart.md
Outdated
{{% alert color="info" %}}**Note**: The output produced by `etcd` are | ||
[logs](../op-guide/configuration/#logging) — info-level logs can | ||
be ignored. {{% /alert %}} | ||
### 1. Installation |
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.
We have this dedicated page for install - https://etcd.io/docs/v3.5/install/ We should try to avoid dual maintenance. I would suggest to make the install page better and provide a link to it from here. That way if anything change with installation steps, we just need to make change to one place.
The check regarding the deployment failed. After investigation: Logs
On my side, |
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.
@ronaldngounou good progress but please see few inline comments. Thanks!
content/en/docs/v3.5/install.md
Outdated
|
||
- [Supported platforms][] | ||
- [Hardware recommendations][] | ||
- [Supported platforms](/docs/v3.5/op-guide/supported-platform) |
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.
Here and other places where you are adding links in bracket. I believe you don't need to that and you can leave what was in the original file. The links at the bottom of the fule takes care of it.
content/en/docs/v3.5/install.md
Outdated
## Install pre-built binaries | ||
|
||
The easiest way to install etcd is from pre-built binaries: | ||
Install `etcd` from pre-built binaries using cURL. |
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 would remove using cURL
explicitly as other tools can be used. I like the steps you provided below.
content/en/docs/v3.5/install.md
Outdated
directory created by the previous step to your path. | ||
4. From a shell, test that `etcd` is in your path: | ||
```bash | ||
# Download and extract the latest release using cURL |
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.
Here, good idea to add in the comment v3.5.21 as an example as it will not be the latest release always. So something like, "# Download and extract the latest release using cURL. For example, download v3.5.21 as below."
content/en/docs/v3.5/quickstart.md
Outdated
[security]: /docs/{{< param version >}}/op-guide/security | ||
[tuning]: /docs/{{< param version >}}/tuning | ||
[Install]: ../install/ | ||
- If you are a developer: |
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 keep these consistent.
i.e. "- If you are a developer:" vs "If you are an operator or admin:".
/retest |
@ronaldngounou can you rebase this now that your other PR is merged? Also, remember to drop |
Signed-off-by: Ronald Ngounou <[email protected]>
Signed-off-by: Ronald Ngounou <[email protected]>
Signed-off-by: Ronald Ngounou <[email protected]>
Signed-off-by: Ronald Ngounou <[email protected]>
Signed-off-by: Ronald Ngounou <[email protected]>
Signed-off-by: Ronald Ngounou <[email protected]>
Signed-off-by: Ronald Ngounou <[email protected]>
Signed-off-by: Ronald Ngounou <[email protected]>
Signed-off-by: Ronald Ngounou <[email protected]>
Signed-off-by: Ronald Ngounou <[email protected]>
…single new line character Signed-off-by: Ronald Ngounou <[email protected]>
It breaks when a Hugo parameter is in the linked URL. Refer to issue DavidAnson/markdownlint#1619 Signed-off-by: Ivan Valdes <[email protected]>
Signed-off-by: Ronald Ngounou <[email protected]>
Signed-off-by: Ronald Ngounou <[email protected]>
2b532fc
to
4cffd5d
Compare
Approach
This PR contributes to the step (2) of the issue #782.
Improves the Quickstart instructions to be clearer, more beginner-friendly, and platform-specific. Add installation commands, port and data path explanations, endpoint flags, and cleanup steps.
Key Improvements:
- Requirements section added.
- Specifies supported OS and architecture
- Provides explicit commands for installing etcd via pre-built binaries
- Includes --endpoints flag for etcdctl to avoid implicit configuration issues
- Adds shutdown instructions
- Maintains fast-start approach while being easier to grasp for new users
As a result, this make the starter page more beginner friendly.
Testing
npm run serve