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

docs: Fix issues found in Queeny's test #507

Merged
merged 24 commits into from
May 24, 2019

Conversation

AstroProfundis
Copy link
Contributor

@AstroProfundis AstroProfundis commented May 21, 2019

What problem does this PR solve?

Make the documents more clear to newbies.

What is changed and how it works?

Fix various issues found in Queeny's test

Check List

Tests

  • Manual test (add detailed scripts or steps below)
  • No code

Code changes

  • Has documents change

Side effects

  • N/A

Related changes

#502

@AstroProfundis AstroProfundis requested review from tennix and lilin90 May 21, 2019 03:17
@AstroProfundis
Copy link
Contributor Author

Please comment if I missed anything

deploy/aws/README.md Show resolved Hide resolved

It may take some while to finish destroying the cluster.

```shell
``` shell
Copy link
Member

Choose a reason for hiding this comment

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

@lilin90 Is this space necessary?

docs/local-dind-tutorial.md Outdated Show resolved Hide resolved
AstroProfundis and others added 2 commits May 22, 2019 10:45

# Or, download binary for macOS
curl -o aws-iam-authenticator https://amazon-eks.s3-us-west-2.amazonaws.com/1.12.7/2019-03-27/bin/darwin/amd64/aws-iam-authenticator

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe using the code below is simpler and more general

os=$(uname -s| tr '[:upper:]' '[:lower:]')
curl -o aws-iam-authenticator https://amazon-eks.s3-us-west-2.amazonaws.com/1.12.7/2019-03-27/bin/$os/amd64/aws-iam-authenticator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have any mac for test, if you can confirm this works for macOS?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it works for macOS and Linux

Copy link
Member

Choose a reason for hiding this comment

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

@AstroProfundis The link you mentioned already has an elegant way to install aws-iam-authenticator.
For macOS, install with homebrew. For linux install with the curl to fetch the binary. It's inappropriate to provide the tricky script in the guide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think homebrew is available to every macOS installation by default...

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it's not builtin on macOS. But almost all technical users install that. So I think provide homebrew command is reasonable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This definitely can't pass Queeney's test, the "almost all technical users" is not fitting the target user groups.

Copy link
Contributor

@yikeke yikeke May 23, 2019

Choose a reason for hiding this comment

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

You can add a note in the document that before users install aws-iam-authenticator with homebrew, they need to make sure that they have installed homebrew first, then providing the homebrew way is totally acceptable for queeny's test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then we're introducing new dependency and copying almost all major parts of AWS document, what's the point of avoiding out-linking again?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, you are right. So if the curl way does not require any new dependency for macOS, I suggest we introduce the curl way in our document then. If users prefer the homebrew way, they can still click the link and find the guide in the AWS document.

deploy/aws/README.md Outdated Show resolved Hide resolved
deploy/aws/README.md Outdated Show resolved Hide resolved
deploy/aws/README.md Outdated Show resolved Hide resolved
deploy/aws/README.md Outdated Show resolved Hide resolved
deploy/aws/README.md Outdated Show resolved Hide resolved
deploy/aws/README.md Outdated Show resolved Hide resolved
deploy/aws/README.md Outdated Show resolved Hide resolved
deploy/aws/README.md Outdated Show resolved Hide resolved
deploy/aws/README.md Outdated Show resolved Hide resolved
deploy/aws/README.md Outdated Show resolved Hide resolved
deploy/aws/README.md Outdated Show resolved Hide resolved
deploy/aws/README.md Outdated Show resolved Hide resolved
deploy/aws/README.md Outdated Show resolved Hide resolved
deploy/aws/README.md Outdated Show resolved Hide resolved
deploy/aws/README.md Outdated Show resolved Hide resolved
deploy/aws/README.md Outdated Show resolved Hide resolved
deploy/aws/README.md Outdated Show resolved Hide resolved
deploy/aws/README.md Show resolved Hide resolved
deploy/aws/README.md Show resolved Hide resolved
AstroProfundis and others added 2 commits May 23, 2019 13:04
Co-Authored-By: Keke Yi <40977455+yikeke@users.noreply.github.com>
@AstroProfundis AstroProfundis force-pushed the fix-queeny-test-issues branch from 100fdce to 7b46590 Compare May 23, 2019 06:09
deploy/aws/README.md Outdated Show resolved Hide resolved
docs/local-dind-tutorial.md Outdated Show resolved Hide resolved
docs/local-dind-tutorial.md Outdated Show resolved Hide resolved
docs/local-dind-tutorial.md Outdated Show resolved Hide resolved
docs/local-dind-tutorial.md Outdated Show resolved Hide resolved
docs/local-dind-tutorial.md Outdated Show resolved Hide resolved
docs/local-dind-tutorial.md Outdated Show resolved Hide resolved
docs/local-dind-tutorial.md Outdated Show resolved Hide resolved
docs/local-dind-tutorial.md Outdated Show resolved Hide resolved
docs/local-dind-tutorial.md Outdated Show resolved Hide resolved
Copy link
Member

@tennix tennix left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@yikeke yikeke left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks a lot for your support during the test. @AstroProfundis

@AstroProfundis AstroProfundis merged commit 2130f66 into pingcap:master May 24, 2019
@AstroProfundis AstroProfundis deleted the fix-queeny-test-issues branch May 24, 2019 08:45
@AstroProfundis AstroProfundis self-assigned this May 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants