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 diagrams to explain all the "requesting certificates" flows #1289

Merged
merged 2 commits into from
Oct 17, 2023

Conversation

inteon
Copy link
Member

@inteon inteon commented Sep 7, 2023

I split the diagram from https://cert-manager.io/docs/concepts/certificate/ into multiple diagrams and placed them on the different "requesting certificates" pages.

@jetstack-bot jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 7, 2023
@netlify
Copy link

netlify bot commented Sep 7, 2023

Deploy Preview for cert-manager-website ready!

Name Link
🔨 Latest commit a52cf3f
🔍 Latest deploy log https://app.netlify.com/sites/cert-manager-website/deploys/652e9fa778c17400086f978e
😎 Deploy Preview https://deploy-preview-1289--cert-manager-website.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.

@inteon inteon force-pushed the add_diagrams branch 3 times, most recently from adbd70a to 83fbd6e Compare September 8, 2023 07:26
@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 21, 2023
@jetstack-bot jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 21, 2023
@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 29, 2023
@jetstack-bot jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 30, 2023
@inteon inteon force-pushed the add_diagrams branch 2 times, most recently from 853b083 to 1be8ac0 Compare October 2, 2023 15:37
@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 6, 2023
@jetstack-bot jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 12, 2023
@inteon inteon changed the title Add diagrams to explain all the "requesting certificates"/ "obtaining certificates" flows Add diagrams to explain all the "requesting certificates" flows Oct 16, 2023
@jetstack-bot jetstack-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 17, 2023
Comment on lines 9 to 11
<div style={{textAlign: "center"}}>
<object data="/images/request-certificate-overview/request-certificate-certificaterequest.svg"></object>
</div>
Copy link
Member

@maelvls maelvls Oct 17, 2023

Choose a reason for hiding this comment

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

I'm unsure about these diagrams. Unless the reader is comfortable with all the terms (CSI driver, etc), I think they won't find any value in looking at the diagram. At best, they will think "huh?", at worst they will feel they are on the wrong page.

What idea are you trying to convey with these diagrams? That they could be using another approach? Or are you reminding them of all the features of cert-manager?

Copy link
Member

@maelvls maelvls Oct 17, 2023

Choose a reason for hiding this comment

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

IMO, this diagram should only show the "resources at play": the CertificateResource + the Secret.

Copy link
Member Author

@inteon inteon Oct 17, 2023

Choose a reason for hiding this comment

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

It aims to show you what resources are created by the user & what resources are auto-generated for each of the different paths. (eg. If you annotate an Ingress, that will create a Certificate resource and that Certificate resource will create CertificateRequest resources.

Copy link
Member

@maelvls maelvls Oct 17, 2023

Choose a reason for hiding this comment

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

IMO, this diagram should only show the "resources at play": the Gateway (with the principal annotation that enables the workflow) + the CertificateResource + the Secret. Anything else will confuse people.

Knowing what resources are at play is important since folks will need to know about them almost immediately to debug things.

Copy link
Member Author

Choose a reason for hiding this comment

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

Knowing what resources are at play is important since folks will need to know about them almost immediately to debug things.
That is my goal for this diagram.
Do you know how we can adapt these diagrams to make that more clear?

Copy link
Member

@maelvls maelvls Oct 17, 2023

Choose a reason for hiding this comment

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

I would probably only show one box with a Gateway, one box with the CertificateResource, and one box with the Secret. I don't think that "CSI driver" and "Certificate" are useful context here

@jetstack-bot jetstack-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 17, 2023
@maelvls
Copy link
Member

maelvls commented Oct 17, 2023

Thank you for digging into my old diagrams and for fixing them!! The new CR diagrams should also help.

/lgtm
/approve

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 17, 2023
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maelvls

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jetstack-bot jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 17, 2023
@jetstack-bot jetstack-bot merged commit e37e27f into cert-manager:master Oct 17, 2023
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants