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

fix: update internal certificate documents #141

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

holyspectral
Copy link
Contributor

Update internal certificate related document.

Note: This should only be merged after 5.4.2 is released.

Copy link
Contributor

@sunilarjun sunilarjun left a comment

Choose a reason for hiding this comment

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

Thank you for updating! I left some suggestions on adjusting the note phrasing to use active voice and to use the "important" admonition instead of "note" as I think it conveys the severity better. Also I left some overall suggestions, and one question but should the note also be included in the version 5.3 page (versioned_docs/version-5.3/02.deploying/01.production/04.internal/04.internal.md)?

Please let me know if there are any questions, thanks again!

@holyspectral
Copy link
Contributor Author

should the note also be included in the version 5.3 page (versioned_docs/version-5.3/02.deploying/01.production/04.internal/04.internal.md)?

Yes we should...thanks for catching that!

@holyspectral holyspectral force-pushed the update-internal-certificates-5-4 branch from 58088cb to 1d845b6 Compare December 19, 2024 19:38
@holyspectral
Copy link
Contributor Author

@sunilarjun I've merged your suggestion into the branch with one question. Feel free to let me know if you have any questions. Thanks!

@holyspectral
Copy link
Contributor Author

@sunilarjun I've updated the PR. Please let me know if any change is still required.

Copy link
Contributor

@sunilarjun sunilarjun left a comment

Choose a reason for hiding this comment

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

Thank you for addressing the changes! Approving but will only merge once 5.4.2 is released.

@holyspectral
Copy link
Contributor Author

@williamshen9999 please also take a look at the doc and let me know if you have any feedback.

@williamshen9999
Copy link
Contributor

@williamshen9999 please also take a look at the doc and let me know if you have any feedback.

Sure, I'm checking it now.

@williamshen9999
Copy link
Contributor

it looks good to me. thanks.

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.

4 participants