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

[TT-13088] Update documentation for master #5412

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

buger
Copy link
Member

@buger buger commented Sep 17, 2024

Triggered by: lghiur

Included:

Tyk Gateway: true
Tyk Dashboard: true
Tyk MDCB false
Tyk Pump false

Intended for: master
Changes sourced from: release-5.6.0
Config info generator branch: main

Note: Updated docs for 5.6.0 (branch suffix: docs)

JIRA: https://tyktech.atlassian.net/browse/TT-13088

Copy link
Contributor

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Key issues to review

Configuration Clarity
The new configuration examples for mutual TLS in 'gateway-config.md' are more detailed but could potentially confuse users with the mix of JSON and environment variable formats. Consider adding separate clear sections or comments to distinguish between these configuration methods to enhance clarity.

Schema Definition
The new schema components for session endpoints and rate limits in 'gateway-swagger.yml' are added, but there is a missing title for the 'RateLimit' schema which might be required for consistency and clarity in the documentation.

Copy link
Contributor

github-actions bot commented Sep 17, 2024

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Best practice
Clarify the importance of unique group IDs across different clusters

To improve clarity, consider rephrasing the description of
TYK_GW_SLAVEOPTIONS_GROUPID to explicitly state the consequences of not maintaining
unique group IDs across different clusters.

tyk-docs/content/shared/gateway-config.md [620]

-The group ID must be the same across all the Gateways of a data-center/cluster which are also sharing the same Redis instance.
+Ensure the group ID is consistent across all Gateways within the same data-center/cluster sharing a Redis instance. Different clusters must have unique group IDs to prevent configuration conflicts.
 
Suggestion importance[1-10]: 9

Why: The suggestion significantly improves clarity on the importance of unique group IDs, which is essential for preventing configuration conflicts across clusters.

9
Possible issue
Correct the spelling in the description of the hashed parameter

Correct the spelling of 'none' to 'any' in the description of the hashed parameter
to improve clarity and correctness.

tyk-docs/assets/others/gateway-swagger.yml [891-893]

 hashed:
   type: string
-  description: Use the hash of the key as input instead of the full key. Any none empty string will be interpreted as to say you want to use hash input.
+  description: Use the hash of the key as input instead of the full key. Any non-empty string will be interpreted as indicating that you want to use hash input.
 
Suggestion importance[1-10]: 8

Why: Correcting the spelling from 'none' to 'non' in the description of the hashed parameter improves clarity and correctness, which is crucial for accurate API documentation.

8
Security
Improve the description for certificate configuration to enhance security awareness

Consider clarifying the description for TYK_GW_SECURITY_CERTIFICATES_UPSTREAM to
include information about the format and security implications of using certificate
IDs or paths. This helps ensure that users are aware of the correct usage and
potential risks.

tyk-docs/content/shared/gateway-config.md [94]

-Upstream is used to specify the certificates to be used in mutual TLS connections to upstream services. These are set at gateway level as a map of domain -> certificate id or path.
+Upstream is used to specify the certificates for mutual TLS connections to upstream services, formatted as a domain-to-certificate mapping (domain -> certificate ID or path). Ensure certificates are managed securely to prevent unauthorized access.
 
Suggestion importance[1-10]: 8

Why: The suggestion enhances security awareness by clarifying the format and potential risks associated with certificate configuration, which is crucial for preventing unauthorized access.

8
Enhancement
Add a meaningful title to the RateLimit schema

Ensure that the title field in the RateLimit schema is not left empty. Providing a
meaningful title helps in understanding the purpose of the schema in the context of
the API documentation.

tyk-docs/assets/others/dashboard-swagger.yml [2435-2450]

 RateLimit:
   properties:
     per:
       type: number
       format: double
       x-go-name: Per
     rate:
       type: number
       format: double
       x-go-name: Rate
-  title: ""
+  title: "Rate Limit Configuration"
 
Suggestion importance[1-10]: 7

Why: Adding a meaningful title to the RateLimit schema improves documentation clarity and helps users understand the schema's purpose, which enhances maintainability and usability.

7
Provide a description for the SessionEndpoint schema

Add a description to the SessionEndpoint schema to provide clarity on its purpose
and usage within the API.

tyk-docs/assets/others/dashboard-swagger.yml [2413-2425]

 SessionEndpoint:
   title: SessionEndpoint holds rate limit on endpoint level.
+  description: "Defines rate limits applicable to specific API endpoints, allowing for granular control over API usage."
   type: object
   properties:
     path:
       type: string
       x-go-name: Path
     methods:
       type: array
       x-go-name: Methods
       items:
         $ref: '#/components/schemas/SessionEndpointMethod'
 
Suggestion importance[1-10]: 7

Why: Adding a description to the SessionEndpoint schema provides clarity on its purpose and usage, which aids developers in understanding the API's structure and functionality.

7
Add a description to the SessionEndpointMethod schema

Add a description to the SessionEndpointMethod schema to enhance the understanding
of its functionality and usage.

tyk-docs/assets/others/dashboard-swagger.yml [2426-2434]

 SessionEndpointMethod:
   title: SessionEndpointMethod holds rate limit on endpoint method level.
+  description: "Specifies rate limits for individual HTTP methods on a given endpoint, allowing differentiated handling based on the method type."
   type: object
   properties:
     name:
       x-go-name: Name
       type: string
     limit:
       x-go-name: Limit
       $ref: '#/components/schemas/RateLimit'
 
Suggestion importance[1-10]: 7

Why: Including a description for the SessionEndpointMethod schema enhances understanding of its functionality and usage, contributing to better documentation and developer experience.

7
Expand the log format description to outline the benefits of each option

The explanation for TYK_GW_LOGFORMAT could be enhanced by specifying the advantages
of each log format option, helping users choose based on their needs.

tyk-docs/content/shared/gateway-config.md [1612]

-You can now configure the log format to be either the standard or json format
+Configure the log format to be either 'standard' for human-readable logs or 'json' for structured logs, which are easier to parse programmatically. Choose based on your monitoring and debugging practices.
 
Suggestion importance[1-10]: 6

Why: While the suggestion provides useful information about log format options, it addresses a minor enhancement rather than a critical improvement.

6
Performance
Add a performance impact note to the disable_dashboard_zeroconf setting

It's recommended to add a note about the potential performance impact when enabling
disable_dashboard_zeroconf. This will help users make informed decisions based on
their network setup and requirements.

tyk-docs/content/shared/gateway-config.md [558]

-In some specific cases, for example, when the Dashboard is bound to a public domain, not accessible inside an internal network, or similar, `disable_dashboard_zeroconf` can be set to `true`, in favor of directly specifying a Tyk Dashboard address.
+In specific cases, such as when the Dashboard is on a public domain not accessible internally, set `disable_dashboard_zeroconf` to `true` to specify the Tyk Dashboard address directly. Note: This may impact network performance depending on your configuration.
 
Suggestion importance[1-10]: 7

Why: Adding a note about performance impact is beneficial for users to make informed decisions, although it addresses a minor issue rather than a critical one.

7

Copy link

netlify bot commented Sep 17, 2024

PS. Pls add /docs/nightly to the end of url

Name Link
🔨 Latest commit 0320ee5
🔍 Latest deploy log https://app.netlify.com/sites/tyk-docs/deploys/66f6be06f3372d00082b4fe9
😎 Deploy Preview https://deploy-preview-5412--tyk-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@buger buger force-pushed the update/TT-13088/release-master-docs branch 2 times, most recently from e842bbd to cf0e8e4 Compare September 18, 2024 11:10
@letzya letzya force-pushed the update/TT-13088/release-master-docs branch from cf0e8e4 to 2deaf2b Compare September 26, 2024 19:41
@letzya letzya force-pushed the update/TT-13088/release-master-docs branch from 2deaf2b to 0320ee5 Compare September 27, 2024 14:15
@letzya
Copy link
Collaborator

letzya commented Sep 27, 2024

@lghiur @Keithwachira is this PR approved? does it contain exactly what you'd expect?

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.

3 participants