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: update "Getting started" guide #1247

Merged
merged 4 commits into from Aug 18, 2022

Conversation

pottekkat
Copy link
Contributor

Signed-off-by: Navendu Pottekkat navendupottekkat@gmail.com

Updates the "Getting started" page. Also updates the ACK installation. Once these changes are approved, it will be propagated to other installation docs as well. Also rearranges installation to the top.

Signed-off-by: Navendu Pottekkat <navendupottekkat@gmail.com>
@pottekkat
Copy link
Contributor Author

@tao12345666333 @juzhiyuan Please review this PR.

@pottekkat
Copy link
Contributor Author

No clue why the markdown link checks are failing.

Copy link
Member

@juzhiyuan juzhiyuan left a comment

Choose a reason for hiding this comment

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

Others LGTM

| `0.5` | `>= 2.4`, `2.5` is recommended. |
| `0.4` |`>= 2.4`|
| APISIX ingress controller | Recommended APISIX version |
| ------------------------: | -------------------------: |
Copy link
Member

Choose a reason for hiding this comment

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

Hello, after adding Recommended in L123, the meaning is changed in my opinion. 🤔 Like in L125, it means >= 2.7 is recommended.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. You are right. I will split this into two columns. I overlooked what is actually mentioned in the column.

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.

@juzhiyuan
Copy link
Member

No clue why the markdown link checks are failing.

@navendu-pottekkat Just rerun, the report says upstream link is 504 😅 but it works now.

@pottekkat
Copy link
Contributor Author

No clue why the markdown link checks are failing.

@navendu-pottekkat Just rerun, the report says upstream link is 504 😅 but it works now.

@juzhiyuan I do not have access to re-run workflows!

Signed-off-by: Navendu Pottekkat <navendupottekkat@gmail.com>
Copy link
Member

@tao12345666333 tao12345666333 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

@tao12345666333 tao12345666333 added the documentation Improvements or additions to documentation label Aug 17, 2022
groupId="resources"
defaultValue="apisix"
values={[
{label: 'APISIX custom resource', value: 'apisix'},
Copy link
Member

Choose a reason for hiding this comment

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

Should be APISIX ingress custom resource.

@pottekkat
Copy link
Contributor Author

pottekkat commented Aug 17, 2022

These failing lint checks do not seem to be from this PR.

It is from this PR.

Copy link
Member

@tao12345666333 tao12345666333 left a comment

Choose a reason for hiding this comment

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

Please correct these links


APISIX ingress controller defines the CRDs [ApisixRoute](./concepts/apisix_route.md), [ApisixUpstream](./concepts/apisix_upstream.md), [ApisixTlx](concepts/apisix_tls.md), and [ApisixClusterConfig](concepts/apisix_cluster_config.md).

APISIX also supports [service discovery](/docs/apisix/discovery/kubernetes/) through [Kubernetes service](https://kubernetes.io/docs/concepts/services-networking/service/) abstraction.
Copy link
Member

Choose a reason for hiding this comment

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

This link is wrong

- Supports service discovery through Kubernetes Service.
- Out-of-the-box node health check support.
- Supports load balancing based on pods (Upstream nodes).
- Rich [Plugins](/docs/apisix/plugins/batch-requests/) with [custom Plugin](/docs/apisix/plugin-develop/) support.
Copy link
Member

Choose a reason for hiding this comment

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

Ditto


First, your supports and cooperations to make this project better are appreciated.
But before you start, please read [How to Contribute](./contribute.md)
See the [Contribute to APISIX](/docs/general/contributor-guide/) section for details on the contributing flow.
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

@pottekkat
Copy link
Contributor Author

@tao12345666333 These links work! I have been using this reference on APISIX docs. What do you suggest we change this to?

I have tested it by building it locally.

Screen.Recording.2022-08-17.at.10.35.30.PM.mov

@pottekkat
Copy link
Contributor Author

pottekkat commented Aug 17, 2022

Sorry. The error in CI is indeed from this PR. But the link checker is naive. These links would work fine in the actual build. It is much better than linking to the APISIX website manually.

@pottekkat
Copy link
Contributor Author

I think we removed this check from the APISIX repo.

@tao12345666333
Copy link
Member

Sorry. The error in CI is indeed from this PR. But the link checker is naive. These links would work fine in the actual build. It is much better than linking to the APISIX website manually.

Yes. The checks in CI are for this repo.
I think we have two ways:

  1. Remove the check - so everything works fine in the website

  2. Modify the link to be a link with the domain name - this works fine whether viewed via GitHub or the website

Signed-off-by: Navendu Pottekkat <navendupottekkat@gmail.com>
@tao12345666333 tao12345666333 merged commit 1a29306 into apache:master Aug 18, 2022
@pottekkat pottekkat deleted the docs/getting-started/1 branch August 18, 2022 08:12
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
Development

Successfully merging this pull request may close these issues.

None yet

4 participants