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: validator command in the doc and add some context #846

Merged
merged 8 commits into from
Apr 18, 2024

Conversation

romeo4934
Copy link

Problem

some commands in the docs are wrong and some context is missing

Summary of Changes

fixing the command and adding some context

@mergify mergify bot requested a review from a team April 16, 2024 19:45
@romeo4934 romeo4934 changed the title fix: validator command in the doc fix: validator command in the doc and add some context Apr 16, 2024
Copy link

@jstarry jstarry left a comment

Choose a reason for hiding this comment

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

looks good to me but I think ci is going to complain about the line length on line 513

@romeo4934
Copy link
Author

@jstarry comment fixed :)

jstarry
jstarry previously approved these changes Apr 18, 2024
@jstarry jstarry added the CI Pull Request is ready to enter CI label Apr 18, 2024
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Apr 18, 2024
@jstarry jstarry added CI Pull Request is ready to enter CI automerge automerge Merge this Pull Request automatically once CI passes labels Apr 18, 2024
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Apr 18, 2024
software. Refer again to
software with the `sol` user. Refer again to
Copy link

Choose a reason for hiding this comment

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

I'm not sure this modification is necessary. This is just a general note saying to make sure the CLI is installed, no command an earlier note already says that we'll be using the sol user so this seems repetitive

Copy link
Author

@romeo4934 romeo4934 Apr 18, 2024

Choose a reason for hiding this comment

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

Before on this page, you wrote to create a sol user.
https://docs.solanalabs.com/operations/setup-a-validator#sol-user
I believe if you recommand to create a sol user, then installation CLI should be installed by this sol user right?
I was myself confused if I had to install the CLI with the sol user or with the root user following each instructions on this page. I had to ask someone on the discord for clarification.

Copy link

Choose a reason for hiding this comment

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

Ahh I see, I was a little confused based on where you added "with the sol user" in the sentence. At the end there, it seems to be reinforcing that the validator will be run with sol user, not which user to use to install it.

Before on this page, you wrote to create a sol user. https://docs.solanalabs.com/operations/setup-a-validator#sol-user

Yep, and shortly after that bit we have this note

It is a best practice to always run your validator as a non-root user, like the sol user we just created.

Altho it doesn't directly say it, this comment implies that the CLI needs to be accessible to user sol

I believe if you recommand to create a sol user, then installation CLI should be installed by this sol user right? I was myself confused if I had to install the CLI with the sol user or with the root user following each instructions on this page. I had to ask someone on the discord for clarification.

Technically it isn't required, but to keep things simplest, yes, the CLI should be installed with user sol.

Now that I understand your point of view, I'm in favor of it. However, I think it is a little ambiguous where you added the note. How about an extra (but short) sentence:

Suggested change
software. Refer again to
software with the `sol` user. Refer again to
software. For simplicity, install the cli with user `sol`. Refer again to

docs/src/operations/setup-a-validator.md Outdated Show resolved Hide resolved
@anza-team anza-team removed the automerge automerge Merge this Pull Request automatically once CI passes label Apr 18, 2024
@anza-team
Copy link
Collaborator

😱 New commits were pushed while the automerge label was present.

@romeo4934
Copy link
Author

@steviez I added your fix suggestion to the PR :)

@romeo4934
Copy link
Author

@steviez I added your suggestion!

@jstarry jstarry added the CI Pull Request is ready to enter CI label Apr 18, 2024
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Apr 18, 2024
Copy link

@steviez steviez left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!

@steviez steviez merged commit fcabbd8 into anza-xyz:master Apr 18, 2024
11 checks passed
michaelschem pushed a commit to michaelschem/agave that referenced this pull request Apr 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants