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

Add README with CLI usage descriptions #20

Merged
merged 9 commits into from
Dec 29, 2023
Merged

Add README with CLI usage descriptions #20

merged 9 commits into from
Dec 29, 2023

Conversation

kota2and3kan
Copy link
Collaborator

Description

This PR adds a README. At this time, it includes descriptions about the CLI tool only.
This is because I need the document of the CLI tool itself to create a new document on the Helm Chart side.
I think we can add descriptions about the Java library if we need it in the future.

Related issues and/or PRs

N/A

Changes made

Add README.

Checklist

  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation to reflect the changes.
  • Any remaining open issues linked to this PR are documented and up-to-date (Jira, GitHub, etc.).
  • Tests (unit, integration, etc.) have been added for the changes.
  • My changes generate no new warnings.
  • Any dependent changes in other PRs have been merged and published.

Additional notes (optional)

At the moment, I think we do NOT need to add this README to the document side.
This is because we assume that users use Helm Chart to run the Scalar Admin k8s instead of running the CLI tool directly.

In the future, I will create the document how to run Scalar Admin k8s by using Helm Chart in the Helm Chart repository and the document how to perform the backup operation by using Scalar Admin k8s in the scalar-kubernetes repository.
These two documents can help users to run the backup operation. So, I think these documents should be added to the doc site in the future instead of this README.

@kota2and3kan kota2and3kan added the documentation Improvements or additions to documentation label Dec 6, 2023
@kota2and3kan kota2and3kan self-assigned this Dec 6, 2023
@@ -0,0 +1,110 @@
# Scalar Admin k8s

## Usage of the CLI tool
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is an output of the scalar-admin-k8s CLI. I think there is no document that describes each flag of scalar-admin-k8s. This is the main purpose of this PR.

This section describes the scalar-admin-k8s options and I can link to this document in the document of Helm Chart to explain the flags of scalar-admin-k8s.

README.md Outdated
zone ID is case sensitive. Etc/UTC by default.
```

## Run the CLI tool on a Kubernetes environment
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This section explains how to run the scalar-admin-k8s CLI tool manually.
However, I think there are almost no users to use this way.
We assume that users use Helm Chart instead of that.

README.md Outdated
- <TIMEZONE>
```

## Run the CLI tool on a Kubernetes environment by using Helm Chart
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After I release the Helm Chart, I will update this section (add a link to the document of Helm Chart) in the future.
We assume that almost all users use this way for backup operations.

Copy link

@choplin choplin left a comment

Choose a reason for hiding this comment

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

LGTM! I left one minor comment. Please take a look.

-n, --namespace=<namespace>
Namespace that Scalar products you want to pause
are deployed. `default` by default.
-r, --release-name=<helmReleaseName>
Copy link

Choose a reason for hiding this comment

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

To be clear, only -r is required, correct? If so, I think it would be helpful to note that this parameter is required in the parameter description here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To be clear, only -r is required, correct?

Yes. You are right.

If so, I think it would be helpful to note that this parameter is required in the parameter description here.

I see. I agree with adding a note.
I will do it in another PR.
Thank you for your suggestion!

Copy link
Collaborator

@supl supl left a comment

Choose a reason for hiding this comment

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

LGTM.
Thank you!

Copy link
Member

@josh-wong josh-wong left a comment

Choose a reason for hiding this comment

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

I've added some comments and suggestions. PTAL!

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated

## Run the CLI tool on a Kubernetes environment

The `scalar-admin-k8s` CLI tool executes Kubernetes APIs in its internal processes. To run those Kubernetes APIs, you must run the `scalar-admin-k8s` CLI tool as a pod on the Kubernetes environment and you must:
Copy link
Member

@josh-wong josh-wong Dec 7, 2023

Choose a reason for hiding this comment

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

Depending on what we call the product (as mentioned in my previous comment), the product mentions in this paragraph might need to be updated.

Suggested change
The `scalar-admin-k8s` CLI tool executes Kubernetes APIs in its internal processes. To run those Kubernetes APIs, you must run the `scalar-admin-k8s` CLI tool as a pod on the Kubernetes environment and you must:
The `scalar-admin-k8s` CLI tool executes Kubernetes APIs in its internal processes. To run those Kubernetes APIs, you must run the `scalar-admin-k8s` CLI tool as a pod on the Kubernetes environment and then do the following:

Copy link
Collaborator Author

@kota2and3kan kota2and3kan Dec 25, 2023

Choose a reason for hiding this comment

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

@josh-wong
Thank you for your suggestion!

In practice, the content of this section is a steps to deploy Scalar Admin for Kubernetes CLI on the Kubernetes environment. In other words, run CLI tool as a pod is included in the steps. So, I feel and then is a bit not fit in this section.

So, how about the following sentence?

Suggested change
The `scalar-admin-k8s` CLI tool executes Kubernetes APIs in its internal processes. To run those Kubernetes APIs, you must run the `scalar-admin-k8s` CLI tool as a pod on the Kubernetes environment and you must:
The `scalar-admin-for-kubernetes` CLI tool executes Kubernetes APIs in its internal processes. To run those Kubernetes APIs, you must run the `scalar-admin-for-kubernetes` CLI tool as a pod on the Kubernetes environment by following to below steps.

Copy link
Member

Choose a reason for hiding this comment

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

@kota2and3kan Thank you for clarifying that! I assumed the steps were separate from the you must run part since we use and to introduce the steps.

In this case, let's use the following:

Suggested change
The `scalar-admin-k8s` CLI tool executes Kubernetes APIs in its internal processes. To run those Kubernetes APIs, you must run the `scalar-admin-k8s` CLI tool as a pod on the Kubernetes environment and you must:
The `scalar-admin-for-kubernetes` CLI tool executes Kubernetes APIs in its internal processes. To run those Kubernetes APIs, you must run the `scalar-admin-for-kubernetes` CLI tool as a pod on the Kubernetes environment by following the steps below:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@josh-wong
Thank you for your suggestion!
I applied it in 8d5d2a4 .

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@feeblefakie feeblefakie left a comment

Choose a reason for hiding this comment

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

Overall, looking good!
Left one suggestion. PTAL!

@@ -0,0 +1,110 @@
# Scalar Admin k8s

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a few sentences describing what this is?
I think it's helpful even for internal developers.
Something like the following.

scalar-admin-k8s is a tool that creates a paused state for Scalar products (e.g., ScalarDB and ScalarDL) in a Kubernetes environment. We can use such a paused state to take backups easily and consistently across multiple diverse databases.

(This might need to be checked by @josh-wong if it is used.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added such an explanation in 71c433e.

@josh-wong
Could you please also check this new sentence? Thank you!

Copy link
Member

@josh-wong josh-wong Dec 25, 2023

Choose a reason for hiding this comment

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

I was unable to suggest changes to the sentence in a reply to this message, so I added a new comment with my suggestion to the updated sentence.

Copy link
Member

@josh-wong josh-wong left a comment

Choose a reason for hiding this comment

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

I've left a few minor suggestions. Other than that, LGTM! Thank you🙇🏻‍♂️

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@brfrn169 brfrn169 left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

Copy link
Contributor

@feeblefakie feeblefakie left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@feeblefakie feeblefakie merged commit ed9f18a into main Dec 29, 2023
5 checks passed
@feeblefakie feeblefakie deleted the add-readme branch December 29, 2023 04:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants