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

tidb-operator: fix documentation usability issues in GCP document #519

Merged
merged 27 commits into from
May 28, 2019
Merged

tidb-operator: fix documentation usability issues in GCP document #519

merged 27 commits into from
May 28, 2019

Conversation

yikeke
Copy link
Contributor

@yikeke yikeke commented May 24, 2019

What problem does this PR solve?

fix documentation usability issues found in GCP deployment document

What is changed and how it works?

Check List

Tests

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

Code changes

  • Has Helm charts change
  • Has Go code change
  • Has CI related scripts change
  • Has documents change

Side effects

  • Breaking backward compatibility

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation

Does this PR introduce a user-facing change?:


@CLAassistant
Copy link

CLAassistant commented May 24, 2019

CLA assistant check
All committers have signed the CLA.

```bash
gcloud compute ssh bastion --zone <zone>
mysql -h <tidb_ilb_ip> -P 4000 -u root
```

It is possible to interact with the cluster using `kubectl` and `helm` with the kubeconfig file `credentials/kubeconfig_<cluster_name>`. The default `cluster_name` is `my-cluster`, it can be changed in `variables.tf`
It is possible to interact with the cluster using `kubectl` and `helm` with the kubeconfig file `credentials/kubeconfig_<cluster_name>`. The default `cluster_name` is `my-cluster`, it can be changed in `variables.tf`:
Copy link
Contributor

@DanielZhangQD DanielZhangQD May 24, 2019

Choose a reason for hiding this comment

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

Putting the descriptions for helm and kubectl in "access the database" section is not reasonable, suggest adding a new section like "interact with the cluster" or "manage the cluster"?

When you are done, the infrastructure can be torn down by running:

``` shell
Copy link
Contributor

@DanielZhangQD DanielZhangQD May 24, 2019

Choose a reason for hiding this comment

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

All "bash" in above codes, keep it same here?


> *Note*: The service account must have sufficient permissions to create resources in the project. The `Project Editor` primitive will accomplish this.

To set the three environment variables, you can first run `vi ~/.bash_profile` and insert the `export` statements in it. Here is an example in `~/.bash_profile`:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest to replace "insert" with "append", BTW, also need to source the file (source ~/.bash_profile or . ~/.bash_profile) after editing


> *Note*: Incrementing the node count will create a node per GCP availability zones.
> *Note*: Incrementing the node count will create a node per GCP availability zone.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be "available zone"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the variables.tf file, the descriptions for tidb_count, tikv_count and other variables all use "per GCP availability zone", so I think we should unify the wording by using "availability" here.

@yikeke
Copy link
Contributor Author

yikeke commented May 27, 2019

I have updated some examples and descriptions according to other deployment docs (see PR#507). PTAL @DanielZhangQD @jlerche @tennix

The upgrading does not finish immediately. You can run `kubectl --kubeconfig credentials/kubeconfig_<cluster_name> get po -n tidb --watch` to verify that all pods are in `Running` state. Then you can [access the database](#access-the-database) and use `tidb_version()` to see whether the cluster has been upgraded successfully:

```sql
MySQL [(none)]> select tidb_version()\G
Copy link
Contributor

Choose a reason for hiding this comment

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

Is "\G" required?

> *Note*: The service account must have sufficient permissions to create resources in the project. The `Project Editor` primitive will accomplish this.

To set the three environment variables, you can first run `vi ~/.bash_profile`, append the `export` statements to it and run `source ~/.bash_profile`.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can actually type export FOO=var or something like that in the terminal, so setting the environment variables in bash_profile is a little overkill. I would suggest removing the line "To set the three environment variables, you can first run vi ~/.bash_profile, append the export statements to it and run source ~/.bash_profile. "

Copy link
Contributor

Choose a reason for hiding this comment

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

If export in the terminal, after customer quit from the session and then open another session to run terraform, the variables need to be exported again, I think it's not convenient


To set the three environment variables, you can first run `vi ~/.bash_profile`, append the `export` statements to it and run `source ~/.bash_profile`.

Here is an example of `~/.bash_profile`:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Here is an example of `~/.bash_profile`:
For example, in your terminal you can enter:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. PTAL again. @jlerche @DanielZhangQD

jlerche
jlerche previously approved these changes May 28, 2019
Copy link
Contributor

@jlerche jlerche 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
Member

@lilin90 lilin90 left a comment

Choose a reason for hiding this comment

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

LGTM

@tennix tennix merged commit b0b71ca into pingcap:master May 28, 2019
@yikeke yikeke deleted the Fix-documentation-usability-issues-found-in-GCP-deployment-doc branch May 28, 2019 05:08
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.

6 participants